- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 3.3k
 
crypto: tree-hash: fix netsplit in counting func #10124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
**WARNING**: This is technically a hard fork.
Relaxes the maximum length of the hash list passed to `tree_hash` from $2^{28}$ to $2^{s-1} - 1$
for nodes compiled in debug mode, where $s$ is the number of bits in a `size_t`. The maximum number
of non-coinbase transactions allowed in a block is `CRYPTONOTE_MAX_TX_PER_BLOCK` is $2^{28}$.
However, the coinbase transaction hash is also passed to `tree_hash` when calculating the block's ID,
which would trip the modified `assert()` call. This could theoretically cause a network split between
release and debug nodes if the non-coinbase transaction count hit $2^{28}$.
The reason why I argue that this change is okay without going through the "proper" hard fork process
is that I believe the likelihood of consensus rules otherwise allowing $2^{28}$ non-coinbase
transactions in a single block, in the near future due to an expanded dynamic block size, is small.
Problem identified by @kayabaNerve.
    4d27e77    to
    e204080      
    Compare
  
    | 
           I solely raised this to you. It's Cuprate's book identifying the maximum amount of transactions per block is actually this lower value (due to the off-by-one discrepancy) which made me aware.  | 
    
This assumes it will be merged, meaning this shouldn't be merged into monero-oxide master quite yet.
| 
           FWIW, you can use an algorithm which works even if the MSB is set @jeffro256, removing the justification for the assert entirely. In Rust, I wrote:      let mut low_pow_2 = {
        let highest_bit_set = usize::BITS - leaves.len().leading_zeros();
        // This won't underflow as we know _a_ bit is set
        1 << (highest_bit_set - 1)
      };(which is only for the branch where there are more than two leaves. Monero special-cases 0, 1, and 2, hence why we can assume there aren't as many leading zeros as bits). 
 But this is micro-optimizing. I believe you'd need a block which is ~570 GB for the prior assert to trip at 228, much less this one at 231 on 32-bit platforms.  | 
    
| 
           The comments in the existing implementation already note the possibility of using a better algorithm: I don't agree with the rationale in the above comment (the function should document its purpose, at which point, one might as well use a fast implementation), but a re-implementation falls out of scope for this PR.  | 
    
WARNING: This is technically a hard fork.
Relaxes the maximum length of the hash list passed to$2^{28}$  to $2^{s-1} - 1$  for nodes compiled in debug mode, where $s$  is the number of bits in a $2^{28}$ . However, the coinbase transaction hash is also passed to $2^{28}$ .
tree_hashfromsize_t. The maximum number of non-coinbase transactions allowed in a block isCRYPTONOTE_MAX_TX_PER_BLOCKistree_hashwhen calculating the block's ID, which would trip the modifiedassert()call. This could theoretically cause a network split between release and debug nodes if the non-coinbase transaction count hitThe reason why I argue that this change is okay without going through the "proper" hard fork process is that I believe the likelihood of consensus rules otherwise allowing$2^{28}$  non-coinbase transactions in a single block, in the near future due to an expanded dynamic block size, is small.
Problem identified by Cuprate.