This adds an implementation of the ChaCha20Poly1305 AEAD exactly matching
the version specified in RFC8439 section 2.8, including tests and official
test vectors.
Remove the variant of ChaCha20Poly1305 AEAD that was previously added in
anticipation of BIP324 using it. BIP324 was updated to instead use rekeying
wrappers around otherwise unmodified versions of the ChaCha20 stream cipher
and the ChaCha20Poly1305 AEAD as specified in RFC8439.
fa9108f85a refactor: Use reinterpret_cast where appropriate (MarcoFalke)
3333f950d4 refactor: Avoid casting away constness (MarcoFalke)
fa6394dd10 refactor: Remove unused C-style casts (MarcoFalke)
Pull request description:
Using a C-style cast to convert pointer types to a byte-like pointer type has many issues:
* It may accidentally and silently throw away `const`.
* It forces reviewers to check that it doesn't accidentally throw away `const`.
For example, on current master a `const char*` is cast to `unsigned char*` (without `const`), see d23fda0584/src/span.h (L273) . This can lead to UB, and the only reason why it didn't lead to UB is because the return type added back the `const`. (Obviously this would break if the return type was deduced via `auto`)
Fix all issues by adding back the `const` and using `reinterpret_cast` where appropriate.
ACKs for top commit:
darosior:
re-utACK fa9108f85a
hebasto:
re-ACK fa9108f85a.
john-moffett:
ACK fa9108f85a
Tree-SHA512: 87f6e4b574f9bd96d4e0f2a0631fd0a9dc6096e5d4f1b95042fe9f197afc2fe9a24e333aeb34fed11feefcdb184a238fe1ea5aff10d580bb18d76bfe48b76a10
faca9a3d5a test: Avoid intermittent issues due to async events in validationinterface_tests (MarcoFalke)
Pull request description:
Currently the tests have many issues:
* They setup the genesis block, even though it is not needed
* They queue an async `UpdatedBlockTip` even, which causes intermittent issues: https://github.com/bitcoin/bitcoin/issues/28146#issuecomment-1650064645
Fix all issues by trimming down the setup to just `ChainTestingSetup`.
ACKs for top commit:
Crypt-iQ:
tACK faca9a3d5a
Tree-SHA512: 4449040330f89bbaf5ce5b2052417c160b451c373987fdf1069596c07834ed81f0aea1506d53c7d2cd21062b27332d30679285dae194b272fd0cb9ce5ded32cf
07c59eda00 Don't derive secure_allocator from std::allocator (Casey Carter)
Pull request description:
Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`.
(Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.)
ACKs for top commit:
jonatack:
re-ACK 07c59eda00 no change since my previous ACK apart from squashing the commits
achow101:
ACK 07c59eda00
john-moffett:
ACK 07c59eda00 Reviewed and tested. Performance appears unaffected in my environment.
Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
6960c81cbf kernel: Remove Univalue from kernel library (TheCharlatan)
10eb3a9faa kernel: Split ParseSighashString (TheCharlatan)
Pull request description:
Besides the build system changes, this is a mostly move-only change for moving the few UniValue-related functions out of kernel files.
UniValue is not required by any of the kernel components and a JSON library should not need to be part of a consensus library.
ACKs for top commit:
achow101:
ACK 6960c81cbf
theuni:
Re-ACK 6960c81cbf
stickies-v:
re-ACK 6960c81cbf
Tree-SHA512: d92e4cb4e12134c94b517751bd746d39f9b8da528ec3a1c94aaedcce93274a3bae9277832e8a7c0243c13df0397ca70ae7bbb24ede200018c569f8d81103c1da
Affects both secure_allocator and zero_after_free_allocator.
Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`.
Drive-by: Aggressively remove facilities unnecessary since C++11 from both allocators to keep things simple.
This is to (a) avoid repeated lookups into the block index for an entry that
should never change and (b) emphasize that the snapshot base should always
exist when set and not change during the runtime of the program.
Thanks to Russ Yanofsky for suggesting this approach.
Also rewrite CheckBlockIndex() to perform tests on all chainstates.
This increases sanity-check coverage, as any place in our code where we were
invoke CheckBlockIndex() on a single chainstate will now invoke the sanity
checks on all chainstates.
This change also tightens up the checks on setBlockIndexCandidates and
mapBlocksUnlinked, to more precisely match what we aim for even in the presence
of assumed-valid blocks.
When using assumeutxo and multiple chainstates are active, the background
chainstate should consider all HAVE_DATA blocks that are ancestors of the
snapshotted block and that have more work than the tip as potential candidates.
When using assumeutxo, we only need the background chainstate to consider
blocks that are on the chain leading to the snapshotted block.
Note that this introduces the new invariant that we can only have an assumeutxo
snapshot where the snapshotted blockhash is in our block index. Unknown block
hashes that are somehow passed in will cause assertion failures when processing
new blocks.
Includes test fixes and improvements by Andrew Chow and Fabian Jahr.
Once a descriptor is successfully parsed, execute more of its methods.
There is probably still room for improvements by checking for some
invariants, but this is a low hanging fruit that significantly increases
the code coverage of these targets.
This new target focuses on fuzzing the actual descriptor parsing logic
by not requiring the fuzzer to produce valid keys (nor a valid checksum
for that matter).
This should make it much more efficient to find bugs we could introduce
moving forward.
Using a character as a marker (here '%') to be able to search and
replace in the string without having to mock the actual descriptor
parsing logic was an insight from Pieter Wuille.
Separate the notion of which blocks are stored on disk, and what data is in our
block index, from what tip a chainstate might be able to get to. We can use
chainstate-agnostic data to determine when to store a block on disk (primarily,
an anti-DoS set of criteria) and let the chainstates figure out for themselves
when a block is of interest for being a candidate tip.
Note: some of the invariants in CheckBlockIndex are modified, but more work is
needed (ie to move CheckBlockIndex to ChainstateManager, as most of what
CheckBlockIndex is doing is checking the consistency of the block index, which
is outside of Chainstate).
node_context is never null, but if it was, it would lead to a nullptr
dereference in node_context->scheduler. Just use EnsureAnyNodeContext
everywhere for more robust, consistent, and correct code.
dd9633b516 test: wallet, add coverage for watch-only raw sh script migration (furszy)
cc781a2180 descriptor: InferScript, do not return top-level only func as sub descriptor (furszy)
286e0c7d5e wallet: loading, log descriptor parsing error details (furszy)
Pull request description:
Linked to #28057.
Currently, the `InferScript` function returns an invalid descriptor when it tries to infer a p2sh-p2pkh script whose pubkey is not known by the wallet.
This behavior occurs because the inference process bypasses the `pkh` subscript when the pubkey is not contained by the wallet (no pubkey provider), interpreting it as a `sh(addr(ADDR))` descriptor. Then, the failure arises because the `addr()` function is restricted to being used only at the top level.
For reviewers, would recommend to start by examining the functional test to understand the context and the circumstances on which this can result in a fatal error (e.g. during the migration process).
ACKs for top commit:
achow101:
ACK dd9633b516
darosior:
utACK dd9633b516
Tree-SHA512: 61e763206c604c372019d2c36e31684f3dddf81f8b154eb9aba5cd66d8d61bda457ed4e591613eb6ce6c76cf7c3f11764abc6cd727a7c2b6414f1065783be032
e.g. sh(addr(ADDR)) or sh(raw(HEX)) are invalid descriptors.
Making sh and wsh top level functions to return addr/raw descriptors when
the subscript inference fails.
fa6245da60 fuzz: Generate process_message targets individually (MarcoFalke)
fa1471e575 refactor: Remove duplicate allNetMessageTypesVec (MarcoFalke)
Pull request description:
Now that `LIMIT_TO_MESSAGE_TYPE` is a runtime setting after commit 927b001502, it shouldn't hurt to also generate each message type individually. Something similar was done for the `rpc` target in commit cf4da5ec29.
ACKs for top commit:
stickies-v:
re-crACK fa6245da60
brunoerg:
reACK fa6245da60
Tree-SHA512: 8f3ec71bab89781f10820a0e027fcde8949f3333eb19a30315aaad6f90f5167028113cea255b2d60b700da817c7eaac20b7b4c92f931052d7f5c2f148d33aa5a
e8c31f135c tests: Test for bumping single output transaction (Andrew Chow)
4f4d4407e3 test: Test bumpfee reduce_output (Andrew Chow)
7d83502d3d bumpfee: Allow original change position to be specified (Andrew Chow)
Pull request description:
When bumping the transaction fee, we will try to find the change output of the transaction in order to have an output whose value we can modify so that we can meet the target feerate. However we do not always find the change output which can cause us to unnecessarily add an additional output to the transaction. We can avoid this issue by outsourcing the determination of change to the user if they so desire.
This PR adds a `orig_change_pos` option to bumpfee which the user can use to specify the index of the change output.
Fixes#11233Fixes#20795
ACKs for top commit:
ismaelsadeeq:
re ACK e8c31f135c
pinheadmz:
re-ACK e8c31f135c
furszy:
Code review ACK e8c31f13
Tree-SHA512: 3a230655934af17f7c1a5953fafb5ef0d687c21355cf284d5e98fece411f589cd69ea505f06d6bdcf82836b08d268c366ad2dd30ae3d71541c9cdf94d1f698ee
f1d807e383 Add more tests for the BIP21 implementation (Kiminuo)
Pull request description:
This PR is an attempt to make it clear how the current BIP21 implementation behaves in Bitcoin Core. Especially, I'm interested whether one can specify multiple `amount` (`message`, etc.) parameters.
My primary end goal is to answer [this question of mine](https://bitcoin.stackexchange.com/questions/118654/how-to-interpret-bip21-uri-with-amount-specified-twice/) but I figured that maybe it's worth a PR. If not, I'll close the PR.
ACKs for top commit:
MarcoFalke:
lgtm ACK f1d807e383
kevkevinpal:
ACK [f1d807e](f1d807e383)
Tree-SHA512: d287809d47c5cfc667f850927bfd969bd345a996d3d53a4c26ef0ffd29eb75ef53358692a15f9a0493ec9e1c101123b6584572e25f87bcb98ff67f6b6c166de4
7d92b1430a refactor: use Span for SipHash::Write (Sebastian Falbesoner)
Pull request description:
This simple refactoring PR changes the interface for the `SipHash` arbitrary-data `Write` method to take a `Span<unsigned char>` instead of having to pass data and length. (`Span<std::byte>` seems to be more modern, but vectors of `unsigned char` are still used prety much everywhere where SipHash is called, and I didn't find it very appealing having to clutter the code with `Make(Writable)ByteSpan` helpers).
ACKs for top commit:
sipa:
utACK 7d92b1430a
MarcoFalke:
lgtm ACK 7d92b1430a
achow101:
ACK 7d92b1430a
Tree-SHA512: f17a27013c942aead4b09f5a64e0c3ff8dbc7e83fe63eb9a2e3ace8be9921c9cbba3ec67e3e83fbe3332ca941c42370efd059e702c060f9b508307e9657c66f2
