blockchain: Consistent overflow prevention.#3689
Open
davecgh wants to merge 19 commits intodecred:masterfrom
Open
blockchain: Consistent overflow prevention.#3689davecgh wants to merge 19 commits intodecred:masterfrom
davecgh wants to merge 19 commits intodecred:masterfrom
Conversation
davecgh
force-pushed
the
multi_blockchain_consistent_overflow_prevention
branch
2 times, most recently
from
444db38 to
6f5c769
Compare
This reworks the tests in TestTreasuryIsFunctions for the treasury add, treasurybase, and treasury spend identification funcs to make them more comprehensive, correct some that weren't actually testing what they claimed, and make them much more consistent with the other tests throughout the code base. Not only does it perform more comprehensive testing, it reduces the test code by about 42%. In particular: - Use hex to bytes for hard-coded byte slices for some of the globals instead of the much more verbose raw byte slices - Introduce helper functions to create the various components of the transactions - Start with well-formed transactions and modify them for each test instead of building them from scratch every time - Run all identification funcs against all of the transaction types to help ensure none of them are incorrectly detected as any other - Significantly improves readability and adds descriptions to make it clear for people not familiar with the code - Modernize the test formatting - Effectively add more tests overall due to cross testing - Correct test intending to pass stakebase but not treasury add and assert it actually passes the stakebase checks This is part of a larger overall effort to bring the treasury code up to the standards used throughout the rest of the blockchain consensus code.
Now that the updated treasury spend tests cover the fully valid case, there is no benefit to repeating it in another test. This is part of a larger overall effort to bring the treasury code up to the standards used throughout the rest of the blockchain consensus code.
This reworks the treasury spend error tests to use the newly introduced functions that start with a valid treasury spend and then mutates a copy to induce the specific error to test. In the process, it also corrects some tests that weren't actually tsting what they claimed. The result is significantly more readable, provides more comprehensive test coverage, is more consistent with the other tests throughout the code base, and reduces the test code for the relevant tests by about 69%. This is part of a larger overall effort to bring the treasury code up to the standards used throughout the rest of the blockchain consensus code.
This reworks the treasury add error tests to use the newly introduced functions that start with a valid treasury add transaction and then mutates a copy to induce the specific error to test. In the process, it also corrects some tests that weren't actually tsting what they claimed. The result is significantly more readable, provides more comprehensive test coverage, is more consistent with the other tests throughout the code base, and reduces the test code for the relevant tests by about 56%. This is part of a larger overall effort to bring the treasury code up to the standards used throughout the rest of the blockchain consensus code.
This reworks the treasurybase error tests to use the newly introduced functions that start with a valid treasurybase and then mutates a copy to induce the specific error to test. In the process, it also corrects some tests that weren't actually tsting what they claimed. The result is significantly more readable, provides more comprehensive test coverage, is more consistent with the other tests throughout the code base, and reduces the test code for the relevant tests by about 63%. This is part of a larger overall effort to bring the treasury code up to the standards used throughout the rest of the blockchain consensus code.
This cleans up the CheckTAdd method to make it much more consistent with the other code used in consensus throughout the rest of the code base. While there are no known exploitable issues with the func and it has worked well for a while now, it is highly inconsistent with the rest of the consensus code in style and polish and has various other issues. For example: - several of the reported error message are incorrect - most of the error message don't provide very helpful messages and reference internal names that are not visible to users - inconsistent variable names - uses less efficient inverted logic tests - various misleading and inaccurate comments - exported func comment refers to internal func that is not visible in generated documention This is part of a larger overall effort to bring the treasury code up to the standards used throughout the rest of the blockchain consensus code.
This cleans up the CheckTSpend method to make it much more consistent with the other code used in consensus throughout the rest of the code base. While there are no known exploitable issues with the func and it has worked well for a while now, it is highly inconsistent with the rest of the consensus code in style and polish and has various other issues. For example: - several of the reported error message are incorrect - most of the error message don't provide very helpful messages and reference internal names that are not visible to users - inconsistent errors - inconsistent variable names - uses less efficient and harder to read inverted logic tests - various misleading and inaccurate comments This is part of a larger overall effort to bring the treasury code up to the standards used throughout the rest of the blockchain consensus code.
This cleans up the CheckTreasuryBase method to make it much more consistent with the other code used in consensus throughout the rest of the code base. While there are no known exploitable issues with the func and it has worked well for a while now, it is highly inconsistent with the rest of the consensus code in style and polish and has various other issues. For example: - several of the reported error message are incorrect - most of the error message don't provide very helpful messages and reference internal names that are not visible to users - inconsistent variable names - some checks are not in the most logical order - various misleading and inaccurate comments - some checks are not making use of existing funcs This is part of a larger overall effort to bring the treasury code up to the standards used throughout the rest of the blockchain consensus code.
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.
davecgh
force-pushed
the
multi_blockchain_consistent_overflow_prevention
branch
from
6f5c769 to
caac2a1
Compare
davecgh
commented
May 6, 2026
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
caac2a1 to
0598f52
Compare
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
1 participant
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.