Skip to content

Conversation

@jeffro256
Copy link
Contributor

@jeffro256 jeffro256 commented Sep 29, 2025

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 Cuprate.

@jeffro256 jeffro256 changed the title crypto: tree-hash: fix netplit in counting func crypto: tree-hash: fix netsplit in counting func Sep 29, 2025
**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.
@jeffro256 jeffro256 force-pushed the fix_tree_hash_netsplit branch from 4d27e77 to e204080 Compare September 29, 2025 02:23
@kayabaNerve
Copy link
Contributor

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.

kayabaNerve added a commit to kayabaNerve/monero-oxide that referenced this pull request Sep 29, 2025
This assumes it will be merged, meaning this shouldn't be merged into
monero-oxide master quite yet.
@kayabaNerve
Copy link
Contributor

kayabaNerve commented Oct 2, 2025

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).

leading_zeros is an intrinsic so this shouldn't be a concern about performance.

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.

@iamamyth
Copy link

iamamyth commented Oct 7, 2025

The comments in the existing implementation already note the possibility of using a better algorithm:

	// This algo has some bad history but all we are doing is 1 << floor(log2(count))
	// There are _many_ ways to do log2, for some reason the one selected was the most obscure one,
	// and fixing it made it even more obscure.
	//
	// Iterative method implemented below aims for clarity over speed, if performance is needed
	// then my advice is to use the BSR instruction on x86

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants