blockchain: Consistent overflow prevention.#3689
Open
davecgh wants to merge 10 commits into
Open
Conversation
davecgh
force-pushed
the
multi_blockchain_consistent_overflow_prevention
branch
3 times, most recently
from
6f5c769 to
caac2a1
Compare
davecgh
commented
May 6, 2026
davecgh
force-pushed
the
multi_blockchain_consistent_overflow_prevention
branch
from
caac2a1 to
0598f52
Compare
davecgh
force-pushed
the
multi_blockchain_consistent_overflow_prevention
branch
from
0598f52 to
91b17a2
Compare
jholdstock
reviewed
May 18, 2026
|
|
||
| // addUnsigned returns the sum of the two unsigned ints of the same size and | ||
| // whether or not the result is safe to use (aka no overflow occurred). | ||
| func addUnsigned[T ~uint16 | ~uint32 | ~uint64](a, b T) (T, bool) { |
Member
There was a problem hiding this comment.
I'm curious why you decided to return a bool from the new funcs instead of an error.
An error string like addition of <a> and <b> overflowed/underflowed could be very helpful for debugging.
Member
Author
There was a problem hiding this comment.
I wanted to leave the error messages up to the caller so they can be more explicit. Otherwise, the error messages would end up generic. With the flag, all of the error messages are explicit and say things like yes vote for treasury spend %v at height %d causes yes count to overflow.
It also makes sure the callers can assign programmatically detectable errors to each case which I prefer for better testability.
This performs some light cleanup of the checkProofOfStake function to make it more consistent with the rest of the code make the error messages more accurate and useful.
Currently, all overflow detection is done inline in multiple places throughout the blockchain code. It would be more ergonomic, consistent, and less error prone to instead use well-tested funcs dedicated to that purpose. To pave the way, this adds two new functions for adding arguments with a returned flag that indicates whether the result is safe to use (that is no overflow or underflow occurred). One variant is for unsigned ints (uint16, uint32, and uint64) and the other is for signed ints (int16, int32, and int64). It only introduces the funcs and does not modify any code to use them.
This adds comprehensive tests for the new addUnsigned and addSigned funcs for all supported types.
Consensus code should ideally always use types of a specific size so there is no possibility of divergent behavior when compiled on different architectures due to differing upper bounds. Further, unsigned types for values that can never be negative should always be preferred. The current signature operations counting code does not adhere to that practice and uses plain ints which means the maximum possible value technically changes depending on the architecture. While this is not currently an issue due to a combination of various limits making it impossible to get anywhere near the limits of the smallest supported architecture (32-bit) and overflow detection, these types of hidden assumptions can easily lead to bugs over time as new features are introduced. This remedies that by modifying the consensus level signature operation counting to use an fixed unsigned 32-bit integer instead of an architecture dependent signed integer. Ideally, the return types on the underlying counting funcs in the script engine would be updated to match and avoid the need to cast, but that would require a major API version bump since it is a public module, so this limits the changes to the internal blockchain, mempool, and mining packages. While here, it also switches to the new consolidated add funcs with cleaner overflow detection versus the current more ad-hoc inline detection. Note that there is no risk of consensus divergence due to the aforementioned impossibility of hitting the conditions with the current combination of parameters and limits.
As mentioned in the previous commit, consensus code should ideally always use types of a specific size so there is no possibility of divergent behavior when compiled on different architectures due to differing upper bounds. Further, unsigned types for values that can never be negative should always be preferred. The current code for counting treasury spend votes does not adhere to practice and uses plain signed integers. It is worth noting that this is not an active issue at the moment because the maximum possible counts achievable due to other limits imposed on the max number of votes and the window over which the number of votes are counted are less than the maximum value of the smallest supported architecture (32 bits). Nevertheless, these types of hidden assumptions are fragile and can easily lead to undetected and unexpected behavior when parameters change and new features are introduced. The primary goal of this commit is to address that by modifying the relevant code to use fixed unsigned 32-bit integers instead. While here, it also takes the opportunity to cleanup the code to make it more consistent with the other consensus code. Note that there is no risk of consensus divergence due to the aforementioned impossibility of hitting the conditions with the current combination of parameters and limits.
The requirement that treasurybases have zero fee is currently indirectly enforced by ensuring the input sum is the required subsidy and that the total of all fees paid in the stake tree do not exceed the input sum. While that doesn't cause any real issues, it's not very clear and it ideally should be done in the per-transaction input checks so it is consistent with all other transaction types. This modifes CheckTransactionInputs to apply the additional input versus output sum check to apply to treasurybase transactions as well.
Currently, enforcement that treasury spends commit to the amount they are spending in the first output and that the amount matches the value specified as the input amount in the first input happen when blocks are connected. The check is not dependent on the current treasury balance or anything that would require it to be limited to block connection only. It should ideally be in the per-transaction input checks so that it is also enforced early when a treasury spend is added to the mempool. To accomplish that, this moves that check, along with a couple of other additional sanity checks, into a separate method dedicated to checking treasury spend inputs so that it is more consistent with the other stake transaction type handling and invokes that method from CheckTransactionInputs. It also moves the other treasury spend checks after the call that checks and connects transactions in the stake tree to ensure the commitment is still verified prior to the other checks that depend on it.
This adds a couple new tests to help ensure the treasurybase overall amount sum semantics are correct.
The current code recalculates the total stake tree fees via a separate getStakeTreeFees function in order to pass them to the regular tree transaction and connection checks so they are accumulated as part of the total overall fees. This behavior is correct, but the total fees are already calculated when checking and connecting the stake tree transactions just before that, so there is no reason to calculate them again when they can simply be returned to the caller. This modifies checkTransactionsAndConnect accordingly and removes the separate method which is no longer necessary.
This updates the transaction input and fee summing code to make use of the new consolidated add funcs with cleaner overflow detection. It also consolidates the input summing for all transaction types into a new closure instead of repeating the logic multiple times throughout the input checks function. Consolidating it makes it more readable and less error prone. Finally, while here, it consolidates, cleans up and slightly optimizes the input sum handling for the transaction types that do not have normal inputs. Namely, first the stakebase summing is changes to use the input value field instead of recalculating the subsidy to make it consistent with the treasurybase and treasury spend cases. This is safe because the code earlier in the function ensures the values matches the expected amounts. Second, it combines the checks for all three types since that change makes the checks for them identical.
davecgh
force-pushed
the
multi_blockchain_consistent_overflow_prevention
branch
from
91b17a2 to
f787e48
Compare
alexlyp
approved these changes
May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This requires
#3677and is rebased on#3679, #3680, and #3688.Currently, all overflow detection is done inline in multiple places throughout the
blockchaincode. It is more ergonomic, consistent, and less error prone to instead use well-tested funcs dedicated to that purpose instead.Further, consensus code should ideally always use types of a specific size so there is no possibility of divergent behavior when compiled on different architectures due to differing upper bounds. In general, unsigned types for values that can never be negative should always be preferred.
Some of the current code does not adhere to that practice and uses plain ints which means the maximum possible value technically changes depending on the architecture.
While these are not currently an issue due to a combination of various limits making it impossible to get anywhere near the limits of the smallest supported architecture (32-bit) and overflow detection, these types of hidden assumptions can easily lead to bugs over time as new features are introduced.
To that end, this consists of a series of commits that add two new functions for adding arguments with a returned flag that indicates whether the result is safe to use (that is no overflow or underflow occurred), convert the existing code to use the new funcs, and change some of the important counting from plain ints to fixed-sized unsigned ints.
Each commit is a self-contained and logically easy to follow change such that the code continues to compile and works properly at each step.
See the description of each commit for further details.