uint256S was previously deprecated for being unsafe. All non-test
usage has already been removed in earlier commits.
1. Tests now use uint256::FromHex() or other constructors wherever
possible without further modification.
2. Tests that can't use uint256::FromHex() because they use input
with non-hex digit characters are
a) modified by dropping the non-hex digit characters if that
provides useful test coverage.
b) dropped if the test without non-hex digit characters does
not provide useful test coverage, e.g. because it is now
duplicated.
Additionally, use BOOST_CHECK_EQUAL where relevant on touched lines
to make error messages more readable.
18d65d2772 test: use uint256::FromUserHex for RANDOM_CTX_SEED (stickies-v)
6819e5a329 node: use uint256::FromUserHex for -assumevalid parsing (stickies-v)
2e58fdb544 util: remove unused IsHexNumber (stickies-v)
8a44d7d3c1 node: use uint256::FromUserHex for -minimumchainwork parsing (stickies-v)
70e2c87737 refactor: add uint256::FromUserHex helper (stickies-v)
85b7cbfcbe test: unittest chainstatemanager_args (stickies-v)
Pull request description:
Since fad2991ba0, `uint256S` has been [deprecated](fad2991ba0 (diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138)) because it is less robust than the `base_blob::FromHex()` introduced in [the same PR](https://github.com/bitcoin/bitcoin/pull/30482). Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. _(see also #30532)_
This PR carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and backwards incompatible behaviour change.
The main behaviour change introduced in this PR is:
- `-minimumchainwork` will raise an error when input is longer than 64 hex digits
- `-assumevalid` will raise an error when input contains invalid hex characters, or when it is longer than 64 hex digits
- test: the optional RANDOM_CTX_SEED env var will now cause tests to abort when it contains invalid hex characters, or when it is longer than 64 hex digits
After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that.
ACKs for top commit:
hodlinator:
re-ACK 18d65d2772
l0rinc:
ACK 18d65d2772
achow101:
ACK 18d65d2772
ryanofsky:
Code review ACK 18d65d2772. Very nice change that cleans up the API, adds checking for invalid values, makes parsing of values more consistent, and adds test coverage.
Tree-SHA512: ec118ea3d56e1dfbc4c79acdbfc797f65c4d2107b0ca9577c848b4ab9b7cb8d05db9f3c7fe8441a09291aca41bf671572431f4eddc59be8fb53abbea76bbbf86
FromUserHex will be used in future commits to construct
uint256 instances from user hex input without being
unnecessarily restrictive on formatting by allowing
0x-prefixed input that is shorter than 64 characters.
Complements uint256::FromHex() nicely in that it naturally does all error checking at compile time and so doesn't need to return an std::optional.
Will be used in the following 2 commits to replace many calls to uint256S(). uint256S() calls taking C-string literals are littered throughout the codebase and executed at runtime to perform parsing unless a given optimizer was surprisingly efficient. While this may not be a hot spot, it's better hygiene in C++20 to store the parsed data blob directly in the binary, without any parsing at runtime.
Follow-up to #30436.
uint256 string representation was wrongfully documented as little-endian due to them being reversed by GetHex() etc, and base_blob::Compare() giving most significance to the beginning of the internal array. They are closer to "big-endian", but this commit tries to be even more precise than that.
uint256_tests.cpp - Avoid using variable from the left side of the condition in the right side.
setup_common.cpp - Skip needless ArithToUint256-conversion.
This is a safe replacement of the previous SetHex, which now returns an
optional to indicate success or failure.
The code is similar to the ParseHashStr helper, which will be removed in
a later commit.
SetHex is fragile, because it accepts any non-hex input or any length of
input, without error feedback. This can lead to issues when the input is
truncated or otherwise corrupted.
Document the problem by renaming the method.
In the future, the fragile method should be removed from the public
interface.
-BEGIN VERIFY SCRIPT-
sed -i 's/SetHex/SetHexDeprecated/g' $( git grep -l SetHex ./src )
-END VERIFY SCRIPT-
fa38d86235 Use only Span{} constructor for byte-like types where possible (MarcoFalke)
fa257bc831 util: Allow std::byte and char Span serialization (MarcoFalke)
Pull request description:
Seems odd to require developers to cast all byte-like spans passed to serialization to `unsigned char`-spans. Fix that by passing and accepting byte-like spans as-is. Finally, add tests and update the code to use just `Span` where possible.
ACKs for top commit:
sipa:
utACK fa38d86235
achow101:
ACK fa38d86235
ryanofsky:
Code review ACK fa38d86235. This looks great. The second commit really removes a lot of boilerplate and shows why the first commit is useful.
Tree-SHA512: 788592d9ff515c3ebe73d48f9ecbb8d239f5b985af86f09974e508cafb0ca6d73a959350295246b4dfb496149bc56330a0b5d659fc434ba6723dbaba0b7a49e5
Prior to this commit it was possible to create base_blobs with any arbitrary amount of bits, like base_blob<9>. One could assume that this would be a valid way to create a bit field that guarantees to have at least 9 bits. However, in such a case, base_blob would not behave as expected because the WIDTH is rounded down to the closest whole byte (simple integer division by 8). This commit makes sure that this oddity is detected and blocked by the compiler.
935acdcc79 refactor: modernize the implementation of uint256.* (pasta)
Pull request description:
- Constructors of uint256 to utilize Span instead of requiring a std::vector
- converts m_data into a std::array
- Prefers using `WIDTH` instead of `sizeof(m_data)`
- make all the things constexpr
- replace C style functions with c++ equivalents
- memset -> std::fill
This may also be replaced by std::memset, but I think that std::fill is more idiomatic of modern c++ and readable.
- memcpy -> std::copy
Note: In practice, implementations of std::copy avoid multiple assignments and use bulk copy functions such as std::memmove if the value type is TriviallyCopyable and the iterator types satisfy LegacyContiguousIterator. (https://en.cppreference.com/w/cpp/algorithm/copy)
This could also likely be replaced by std::memcpy, but as said above, I believe the using std::copy is the more c++ way to do anything and is almost guaranteed to compile to the same asm
- memcmp -> std::memcmp
ACKs for top commit:
achow101:
ACK 935acdcc79
hebasto:
Approach ACK 935acdcc79.
aureleoules:
reACK 935acdcc79
john-moffett:
ACK 935acdcc79
stickies-v:
Approach ACK 935acdcc7
Tree-SHA512: 4f1ba54ff2198eea0e505d41e73d552c84c60f6878d5c85a94a8ab57f39afc94ef8d79258e7afd01fa84ec2a99f4404bb877eecd671f65e1ee9273f3129fc650
- Constructors of uint256 to utilize Span instead of requiring a std::vector
- converts m_data into a std::array
- Prefers using `WIDTH` instead of `sizeof(m_data)`
- make all the things constexpr
- replace C style functions with c++ equivalents
- memset -> std::fill
- memcpy -> std::copy
Note: In practice, implementations of std::copy avoid multiple assignments and use bulk copy functions such as std::memmove if the value type is TriviallyCopyable and the iterator types satisfy LegacyContiguousIterator. (https://en.cppreference.com/w/cpp/algorithm/copy)
- memcmp -> std::memcmp
This adds a new module (unused for now) which defines TxRequestTracker, a data
structure that maintains all information about transaction requests, and coordinates
requests.
Replace the memset with C++11 value/aggregate initialisation of
the m_data array, which still ensures the unspecified values end
up as zero-initialised.
This then allows changing UINT256_ONE() from dynamically allocating an
object, to a simpler referencing a static allocation.
This is in preparation for exposing a ::data member function.
-BEGIN VERIFY SCRIPT-
sed -i "s/\([^.]\|other.\)data/\1m_data/g" src/uint256.h src/uint256.cpp
-END VERIFY SCRIPT-
In the case of CKey's destructor, it seems to have been an oversight in
f4d1fc259 not to delete it. At this point, it results in the move
constructors/assignment operators for CKey being deleted, which may have
a performance impact.
Remove the nType and nVersion as parameters to all serialization methods
and functions. There is only one place where it's read and has an impact
(in CAddress), and even there it does not impact any of the recursively
invoked serializers.
Instead, the few places that need nType or nVersion are changed to read
it directly from the stream object, through GetType() and GetVersion()
methods which are added to all stream classes.
Given that in default GetSerializeSize implementations created by
ADD_SERIALIZE_METHODS we're already using CSizeComputer(), get rid
of the specialized GetSerializeSize methods everywhere, and just use
CSizeComputer. This removes a lot of code which isn't actually used
anywhere.
For CCompactSize and CVarInt this actually removes a more efficient
size computing algorithm, which is brought back in a later commit.
- Update comments in checkpoints to be doxygen compatible
- Update comments in checkqueue to be doxygen compatible
- Update coins to be doxygen compatible
- Fix comment typo in crypter.h
- Update licenses/copyright dates
Closes#5325#5184#5183#5182
