Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Hide the leveldb::Iterator member variable with a pimpl in order not to
expose it directly in the header.
Also, move CDBWrapper::NewIterator to the dbwrapper implementation to
use the pimpl for CDBIterator initialziation.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Hide the leveldb::WriteBatch member variable with a pimpl in order not
to expose it directly in the header.
Also move CDBBatch::Clear to the dbwrapper implementation to use the new
impl_batch.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
Keep the generic serialization in the header, while moving
leveldb-specifics to the implementation file.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
f054bd072a refactor: use "if constexpr" in std::vector's Unserialize() (Martin Leitner-Ankerl)
088caa68fb refactor: use "if constexpr" in std::vector's Serialize() (Martin Leitner-Ankerl)
0fafaca4d3 refactor: use "if constexpr" in prevector's Unserialize() (Martin Leitner-Ankerl)
c8839ec5cd refactor: use "if constexpr" in prevector's Serialize() (Martin Leitner-Ankerl)
1403d181c1 refactor: use fold expressions instead of recursive calls in UnserializeMany() (Martin Leitner-Ankerl)
bd08a008b4 refactor: use fold expressions instead of recursive calls in SerializeMany() (Martin Leitner-Ankerl)
Pull request description:
This simplifies the serialization code a bit and should also make it a bit faster.
* use fold expressions instead of recursive calls. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations.
* use `if constexpr` instead of unnecessarily creating a temporary object only to call the right overload. This is used for `std::vector` and `prevector` serialization.
ACKs for top commit:
MarcoFalke:
only change is to add a missing `&`. lgtm, re-ACK f054bd072a📦
jonatack:
ACK f054bd072a
sipa:
utACK f054bd072a
john-moffett:
ACK f054bd072a
Tree-SHA512: 0417bf2d6be486c581732297945449211fc3481bac82964e27628b38ef55a47dfa58d730148aeaf1b19fa8eb1076489cc646ceebb178162a9afa59034601501d
It is incorrect to assert that `cache.HaveCoin()` will always be `true`
if `backend.HaveCoin()` is. The coin could well have been marked as
spent in the cache but not yet flushed, in which case `cache.HaveCoin()`
would return `false`.
Note this was never hit because `exists_using_have_coin_in_backend` is
currently never `true` (it's the default implementation of `CCoinsView`.
However this might change if we were to add a target where the backend
is a `CCoinsViewDB`.
Diversify outbound connections with respect to
networks: Every ~5 minutes, try to add an extra connection
to a reachable network which we currently don't have a connection to.
This is done defensively - only try management with respect to networks
after all existing outbound slots are filled.
The resulting situation with an extra outbound peer will be handled
by the extra outbound eviction logic, which protects peers from
eviction if they are the only ones for their network.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
If a peer is the only one of its network, protect it from eviction.
This improves the diversity of outbound connections with respect to
reachable networks.
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Connman uses this new map to keep a count of active OUTBOUND_FULL_RELAY and
MANUAL connections. Unused until next commit.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
faafc35a77 doc: Clarify that -datacarriersize applies to the full raw scriptPubKey, not the data push (MarcoFalke)
55550e7fe7 test: Add -datacarriersize=2 tests (MarcoFalke)
Pull request description:
Clarify with a test that `-datacarriersize` applies to the serialized size of the scriptPubKey, not the size of the pushed data. So for example,
* `-datacarriersize=2` will reject a `raw(6a01aa)`, even though only one byte is pushed
* `-datacarriersize=0` (or `-datacarrier=0`) will reject a `raw(6a)`, even though no byte is pushed
* `-datacarriersize=0` (or `-datacarrier=0`) will reject a `raw(6a00)`, even though zero bytes are pushed
ACKs for top commit:
ajtowns:
ACK faafc35a77
instagibbs:
ACK faafc35a77
Tree-SHA512: f01ace02798f596ac2a02461e9f2a6ef91b3b37c976ea0b3bc860e2d3efb0ace0fd8b779dd18249cee7f84ebbe5fd21d8506afd3a15edadc00b843ff3b4aacc7
025fda0a76 fuzz: addrman, avoid `ConsumeDeserializable` when possible (brunoerg)
Pull request description:
Using specific functions like `ConsumeService`, `ConsumeAddress` and `ConsumeNetAddr` may be more effective than using `ConsumeDeserializable`. They always return some value while `ConsumeDeserializable` may return `std::nullopt`.
E.g.: In this part of the code, if `op_net_addr` is `std::nullopt`, we basically generated the addresses (if so) unnecessarily, because we won't be able to use them:
```cpp
std::vector<CAddress> addresses;
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) {
const std::optional<CAddress> opt_address = ConsumeDeserializable<CAddress>(fuzzed_data_provider);
if (!opt_address) {
break;
}
addresses.push_back(*opt_address);
}
const std::optional<CNetAddr> opt_net_addr = ConsumeDeserializable<CNetAddr>(fuzzed_data_provider);
if (opt_net_addr) {
addr_man.Add(addresses, *opt_net_addr, std::chrono::seconds{ConsumeTime(fuzzed_data_provider, 0, 100000000)});
}
```
Also, if we are not calling `Add` effectively, it would also be affect other functions that may "depend" on it.
ACKs for top commit:
dergoegge:
Code review ACK 025fda0a76
Tree-SHA512: 02450bec0b084c15ba0cd1cbdfbac067c8fea4ccf27be0c86d54e020f029a6c749a16d8e0558f9d6d35a7ca9db8916f180c872f09474702b5591129e9be0d192
Rather than using a bloom filter to track announced invs, simply allow
a peer to request any tx that entered the mempool prior to the last INV
message we sent them. This also obsoletes the UNCONDITIONAL_RELAY_DELAY.
Instead of recursively calling `UnserializeMany` and peeling off one
argument at a time, use a fold expression. This simplifies the code,
makes it most likely faster because it reduces the number of function
calls, and compiles faster because there are fewer template
instantiations.
Instead of recursively calling `SerializeMany` and peeling off one
argument at a time, use a fold expression. This simplifies the code,
makes it most likely faster because it reduces the number of function
calls, and compiles faster because there are fewer template
instantiations.
Wrap leveldb::DestroyDB in a helper function without exposing
leveldb-specifics.
Also, add missing optional include.
The context of this commit is an effort to decouple the dbwrapper header
file from leveldb includes. To this end, the includes are moved to the
dbwrapper implementation file. This is done as part of the kernel
project to reduce the number of required includes for users of the
kernel.
fa633aa690 streams: Teach AutoFile how to XOR (MarcoFalke)
000019e158 Add AutoFile::detail_fread member function (MarcoFalke)
fa7724bc9d refactor: Modernize AutoFile (MarcoFalke)
fa8d227d58 doc: Remove comments that just repeat what the code does (MarcoFalke)
fafe2ca0ce refactor: Remove redundant file check from AutoFile shift operators (MarcoFalke)
9999a49b32 Extract util::Xor, Add key_offset option, Add bench (MarcoFalke)
Pull request description:
This allows `AutoFile` to roll an XOR pattern while reading or writing to the underlying file.
This is needed for https://github.com/bitcoin/bitcoin/pull/28052, but can also be used in any other place.
Also, there are tests, so I've split this up from the larger pull to make review easier, hopefully.
ACKs for top commit:
Crypt-iQ:
crACK fa633aa
willcl-ark:
Lightly tested ACK fa633aa690
jamesob:
reACK fa633aa690 ([`jamesob/ackr/28060.4.MarcoFalke.util_add_xorfile`](https://github.com/jamesob/bitcoin/tree/ackr/28060.4.MarcoFalke.util_add_xorfile))
Tree-SHA512: 6d66cad0a089a096d3f95e4f2b28bce80b349d4b76f53d09dc9a0bea4fc1b7c0652724469c37971ba27728c7d46398a4c1d289c252af4c0f83bb2fcbc6f8e90b
bee2d57a65 script: update flake8 to 6.1.0 (Jon Atack)
38c3fd846b test: python E721 updates (Jon Atack)
Pull request description:
Update our functional tests per [E721](https://www.flake8rules.com/rules/E721.html) enforced by [flake8 6.1.0](https://flake8.pycqa.org/en/latest/release-notes/6.1.0.html), and update our CI lint task to use that release. This makes the following linter output on current master with flake8 6.1.0 green.
```
$ ./test/lint/lint-python.py ; ./test/lint/lint-spelling.py
test/functional/p2p_invalid_locator.py:35:16: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
test/functional/test_framework/siphash.py:34:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
test/functional/test_framework/siphash.py:64:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
src/test/fuzz/descriptor_parse.cpp:88: occurences ==> occurrences
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```
ACKs for top commit:
MarcoFalke:
lgtm ACK bee2d57a65
Tree-SHA512: f3788a543ca98e44eeeba1d06c32f1b11eec95d4aef068aa1b6b5c401261adfa3fb6c6d6c769f3fe6839d78e74a310d5c926867e7c367d6513a53d580fd376f3
a733dd79e2 Remove unused function `reliesOnAssumedValid` (Suhas Daftuar)
d4a11abb19 Cache block index entry corresponding to assumeutxo snapshot base blockhash (Suhas Daftuar)
3556b85022 Move CheckBlockIndex() from Chainstate to ChainstateManager (Suhas Daftuar)
0ce805b632 Documentation improvements for assumeutxo (Ryan Ofsky)
768690b7ce Fix initialization of setBlockIndexCandidates when working with multiple chainstates (Suhas Daftuar)
d43a1f1a2f Tighten requirements for adding elements to setBlockIndexCandidates (Suhas Daftuar)
d0d40ea9a6 Move block-storage-related logic to ChainstateManager (Suhas Daftuar)
3cfc75366e test: Clear block index flags when testing snapshots (Suhas Daftuar)
272fbc370c Update CheckBlockIndex invariants for chains based on an assumeutxo snapshot (Suhas Daftuar)
10c05710ce Add wrapper for adding entries to a chainstate's block index candidates (Suhas Daftuar)
471da5f6e7 Move block-arrival information / preciousblock counters to ChainstateManager (Suhas Daftuar)
1cfc887d00 Remove CChain dependency in node/blockstorage (Suhas Daftuar)
fe86a7cd48 Explicitly track maximum block height stored in undo files (Suhas Daftuar)
Pull request description:
This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation.
This PR also fixes a bug in how a chainstate's `setBlockIndexCandidates` was being initialized; it should always have all the HAVE_DATA block index entries that have more work than the chain tip. During startup, we were not fully populating `setBlockIndexCandidates` in some scenarios involving multiple chainstates.
Further, this PR establishes a concept that whenever we have 2 chainstates, that we always know the snapshotted chain's base block and the base block's hash must be an element of our block index. Given that, we can establish a new invariant that the background validation chainstate only needs to consider blocks leading to that snapshotted block entry as potential candidates for its tip. As a followup I would imagine that when writing net_processing logic to download blocks for the background chainstate, that we would use this concept to only download blocks towards the snapshotted entry as well.
ACKs for top commit:
achow101:
ACK a733dd79e2
jamesob:
reACK a733dd79e2 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic))
Sjors:
Code review ACK a733dd79e2.
ryanofsky:
Code review ACK a733dd79e2. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge.
Tree-SHA512: 9ec17746f22b9c27082743ee581b8adceb2bd322fceafa507b428bdcc3ffb8b4c6601fc61cc7bb1161f890c3d38503e8b49474da7b5ab1b1f38bda7aa8668675
ecfe507e07 fuzz: use `ConnmanTestMsg` in `connman` (brunoerg)
Pull request description:
Fixes#27980
Using `ConnmanTestMsg` we can add nodes and be
more effective fuzzing functions like `DisconnectNode`,
`FindNode`, `GetNodeStats` and other ones.
ACKs for top commit:
MarcoFalke:
review ACK ecfe507e07
dergoegge:
utACK ecfe507e07
Tree-SHA512: 97c363b422809f2e9755c082d1102237347abfab72c7baca417bd8975f8a595ddf3a085f8353dbdb9f17fb98fbfe830792bfc0b83451168458018faf6c239efa
fabef121b0 refactor: Use EnsureAnyNodeContext (MarcoFalke)
fa1640617e test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC (MarcoFalke)
Pull request description:
There should be no risk or downside in adding a call to `SyncWithValidationInterfaceQueue` here. In fact, it will make tests less brittle. For example,
* If one sets the timeouts in `test/functional/feature_fee_estimation.py` to `0`, on `master` the test will fail and here it will pass.
* It may avoid a rare (theoretic) intermittent issue in https://github.com/bitcoin/bitcoin/pull/28108/files#r1268966663
ACKs for top commit:
TheCharlatan:
ACK fabef121b0
furszy:
Code review ACK fabef121. Convinced by checking all current tests usages.
Tree-SHA512: c9e9a536a8721d1b3f267a66b40578b34948892301affdcad121ef8e02bf17037305d0dd53aa94b1b064753e66f9cfb31823b916b707a9d812627f502b818003
06199a995f refactor: Revert addition of univalue sighash string check (TheCharlatan)
0b47c16215 doc: Correct release-notes for sighashtype exceptions (TheCharlatan)
Pull request description:
This is a follow up for #28113.
The string type check is already done by the rpc parser / RPCHelpMan. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274568557).
Also correct the release note for the correct sighashtype exception change. There is no change in the handling of non-string sighashtype arugments. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274567555).
ACKs for top commit:
MarcoFalke:
lgtm ACK 06199a995f
jonatack:
Tested ACK 06199a995f
stickies-v:
ACK 06199a995f
Tree-SHA512: 3faa6b3d2247624c0973df8d79c09fbf1f90ffb99f1be484e359b528f485c31affea45976759bd206e4c81cbb54ebba5ad0ef4127d1deacbfe2a58153fcc94ee
fa940f41ea Remove unused raw-pointer read helper from univalue (MarcoFalke)
Pull request description:
The helpers are unused outside of tests and redundant with the existing `bool read(std::string_view raw);`.
Fix both issues by removing them.
Also, simplify the tests code by removing a `std::string` constructor where possible.
ACKs for top commit:
stickies-v:
utACK fa940f41ea
TheCharlatan:
tACK fa940f41ea
Tree-SHA512: 60c154c1046f01551335af79bf820a6104844f63e89977271b4336b3cd06ac3bab1379e18b7bc61d12bef7446029e91c16541ddecf9e88bc8bc897fc1f6ee2c8
131314b62e fuzz: increase coverage of the descriptor targets (Antoine Poinsot)
90a24741e7 fuzz: add a new, more efficient, descriptor parsing target (Antoine Poinsot)
d60229ede5 fuzz: make the parsed descriptor testing into a function (Antoine Poinsot)
Pull request description:
The current descriptor parsing fuzz target requires valid public or private keys to be provided. This is unnecessary as we are only interested in fuzzing the descriptor parsing logic here (other targets are focused on fuzzing keys serializations). And it's pretty inefficient, especially for formats that need a checksum (`xpub`, `xprv`, WIF).
This introduces a new target that mocks the keys as an index in a list of precomputed keys. Keys are represented as 2 hex characters in the descriptor. The key type (private, public, extended, ..) is deterministically based on this one-byte value. Keys are deterministically generated at target initialization. This is much more efficient and also largely reduces the size of the seeds.
TL;DR: for instance instead of requiring the fuzzer to generate a `pk(xpub6DdBu7pBoyf7RjnUVhg8y6LFCfca2QAGJ39FcsgXM52Pg7eejUHLBJn4gNMey5dacyt4AjvKzdTQiuLfRdK8rSzyqZPJmNAcYZ9kVVEz4kj)` to parse a valid descriptor, it just needs to generate a `pk(03)`.
Note we only mock the keys themselves, not the entire descriptor key expression. As we want to fuzz the real code that parses the rest of the key expression (origin, derivation paths, ..).
This is a target i used for reviewing #17190 and #27255, and figured it was worth PR'ing on its own since the added complexity for mocking the keys is minimal and it could help prevent introducing bugs to the descriptor parsing logic much more efficiently.
ACKs for top commit:
MarcoFalke:
re-ACK 131314b62e🐓
achow101:
ACK 131314b62e
Tree-SHA512: 485a8d6a0f31a3a132df94dc57f97bdd81583d63507510debaac6a41dbbb42fa83c704ff3f2bd0b78c8673c583157c9a3efd79410e5e79511859e1470e629118
This check is already done by the rpc parser. Re-doing it is adding dead
code. Instead, throwing an exception when the assumption does not hold
is the already correct behavior.
To make the fuzz test more accurate and not swallow all runtime errors,
add a check that the passed in UniValue sighash argument is either a
string or null.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
This adds the FSChaCha20Poly1305 AEAD as specified in BIP324, a wrapper
around the ChaCha20Poly1305 AEAD (as specified in RFC8439 section 2.8) which
automatically rekeys every N messages, and automatically increments the nonce
every message.
This adds the FSChaCha20 stream cipher as specified in BIP324, a
wrapper around the ChaCha20 stream cipher (specified in RFC8439
section 2.4) which automatically rekeys every N messages, and
manages the nonces used for encryption.
Co-authored-by: dhruv <856960+dhruv@users.noreply.github.com>
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
f6a26196cf Added `longpollid` and `data` params to `template_request` #27998 (Rhythm Garg)
Pull request description:
This PR will add the optional parameters `longpollid` and `data` to `template_request` as they were missing when calling `help getblocktemplate` in RPCHelpMan.
I request the maintainers to review this and let me know about any mistakes in the descriptions of the parameters.
This PR refers to the issue #27998
ACKs for top commit:
ItIsOHM:
> tACK [f6a2619](f6a26196cf)
russeree:
tACK f6a26196cf
stickies-v:
tACK f6a26196cf
Tree-SHA512: 6c592db59cb11b2d031ce5265c547fa296266278f6c25f96afe18a420e0d547f4d483e0f66de75d52c0c319ac1585f3558b9f70c12ef208c96ec96a51f786c6a
5080c9c25f build: adapt Windows builds for libsecp256k1 build changes (fanquake)
ff061fde18 Squashed 'src/secp256k1/' changes from 705ce7ed8c..c545fdc374 (fanquake)
Pull request description:
Includes https://github.com/bitcoin-core/secp256k1/pull/1378. Which fixes#28079.
Adapts Windows build for https://github.com/bitcoin-core/secp256k1/pull/1367.
ACKs for top commit:
hebasto:
ACK 5080c9c25f, I've made the `src/secp256k1` subtree update locally and got zero diff with this PR branch.
jonasnick:
ACK 5080c9c25f
Tree-SHA512: 37915d420ebacefc6bc82c2511bff3d6884e01d5c92795f19cd61862f96b30aa1fe768aeabec128c9d25c1d8bc62b46b46969626067266074b39566ad9e2f5ba
1cd45d4e08 test: move random.h include header from setup_common.h to cpp (Jon Atack)
1b246fdd14 test: move remaining random test util code from setup_common to random (jonatack)
Pull request description:
and drop the `util/random` dependency on `util/setup_common`. This improves code separation and allows `util/setup_common` to call `util/random` functions without creating a circular dependency, thereby addressing https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1497266140 by glozow (thanks!)
ACKs for top commit:
MarcoFalke:
lgtm ACK 1cd45d4e08🌂
Tree-SHA512: 6ce63d9103ba9b04eebbd8ad02fe9aa79e356296533404034a1ae88e9b7ca0bc9a5c51fd754b71cf4e7b55b18bcd4d5474b2d588edee3851e3b3ce0e4d309a93
c545fdc374 Merge bitcoin-core/secp256k1#1298: Remove randomness tests
b40e2d30b7 Merge bitcoin-core/secp256k1#1378: ellswift: fix probabilistic test failure when swapping sides
c424e2fb43 ellswift: fix probabilistic test failure when swapping sides
907a67212e Merge bitcoin-core/secp256k1#1313: ci: Test on development snapshots of GCC and Clang
0f7657d59c Merge bitcoin-core/secp256k1#1366: field: Use `restrict` consistently in fe_sqrt
cc55757552 Merge bitcoin-core/secp256k1#1340: clean up in-comment Sage code (refer to secp256k1_params.sage, update to Python3)
600c5adcd5 clean up in-comment Sage code (refer to secp256k1_params.sage, update to Python3)
981e5be38c ci: Fix typo in comment
e9e9648219 ci: Reduce number of macOS tasks from 28 to 8
609093b387 ci: Add x86_64 Linux tasks for gcc and clang snapshots
1deecaaf3b ci: Install development snapshots of gcc and clang
b79ba8aa4c field: Use `restrict` consistently in fe_sqrt
c9ebca95f9 Merge bitcoin-core/secp256k1#1363: doc: minor ellswift.md updates
afd7eb4a55 Merge bitcoin-core/secp256k1#1371: Add exhaustive tests for ellswift (with create+decode roundtrip)
2792119278 Add exhaustive test for ellswift (create+decode roundtrip)
c7d900ffd1 doc: minor ellswift.md updates
332af315fc Merge bitcoin-core/secp256k1#1344: group: save normalize_weak calls in `secp256k1_ge_is_valid_var`/`secp256k1_gej_eq_x_var`
9e6d1b0e9b Merge bitcoin-core/secp256k1#1367: build: Improvements to symbol visibility logic on Windows (attempt 3)
0aacf64352 Merge bitcoin-core/secp256k1#1370: Corrected some typos
b6b9834e8d small fixes
07c0e8b82e group: remove unneeded normalize_weak in `secp256k1_gej_eq_x_var`
3fc1de5c55 Merge bitcoin-core/secp256k1#1364: Avoid `-Wmaybe-uninitialized` when compiling with `gcc -O1`
fb758fe8d6 Merge bitcoin-core/secp256k1#1323: tweak_add: fix API doc for tweak=0
c6cd2b15a0 ci: Add task for static library on Windows + CMake
020bf69a44 build: Add extensive docs on visibility issues
0196e8ade1 build: Introduce `SECP256k1_DLL_EXPORT` macro
9f1b1904a3 refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API`
ae9db95cea build: Introduce `SECP256K1_STATIC` macro for Windows users
7966aee31d Merge bitcoin-core/secp256k1#1369: ci: Print commit in Windows container
a7bec34231 ci: Print commit in Windows container
249c81eaa3 Merge bitcoin-core/secp256k1#1368: ci: Drop manual checkout of merge commit
98579e297b ci: Drop manual checkout of merge commit
5b9f37f136 ci: Add `CFLAGS: -O1` to task matrix
a6ca76cdf2 Avoid `-Wmaybe-uninitialized` when compiling with `gcc -O1`
0fa84f869d Merge bitcoin-core/secp256k1#1358: tests: introduce helper for non-zero `random_fe_test()` results
5a95a268b9 tests: introduce helper for non-zero `random_fe_test` results
304421d57b tests: refactor: remove duplicate function `random_field_element_test`
3aef6ab8e1 Merge bitcoin-core/secp256k1#1345: field: Static-assert that int args affecting magnitude are constant
4494a369b6 Merge bitcoin-core/secp256k1#1357: tests: refactor: take use of `secp256k1_ge_x_on_curve_var`
799f4eec27 Merge bitcoin-core/secp256k1#1356: ci: Adjust Docker image to Debian 12 "bookworm"
c862a9fb49 ci: Adjust Docker image to Debian 12 "bookworm"
a1782098a9 ci: Force DWARF v4 for Clang when Valgrind tests are expected
7d8d5c86df tests: refactor: take use of `secp256k1_ge_x_on_curve_var`
8a7273465b Help the compiler prove that a loop is entered
fd491ea1bb Merge bitcoin-core/secp256k1#1355: Fix a typo in the error message
ac43613d25 Merge bitcoin-core/secp256k1#1354: Add ellswift to CHANGELOG
67887ae65c Fix a typo in the error message
926dd3e962 Merge bitcoin-core/secp256k1#1295: abi: Use dllexport for mingw builds
10836832e7 Merge bitcoin-core/secp256k1#1336: Use `__shiftright128` intrinsic in `secp256k1_u128_rshift` on MSVC
7c7467ab7f Refer to ellswift.md in API docs
c32ffd8d8c Add ellswift to CHANGELOG
3c1a0fd37f Merge bitcoin-core/secp256k1#1347: field: Document return value of fe_sqrt()
5779137457 field: Document return value of fe_sqrt()
be8ff3a02a field: Static-assert that int args affecting magnitude are constant
efa76c4bf7 group: remove unneeded normalize_weak in `secp256k1_ge_is_valid_var`
5b7bf2e9d4 Use `__shiftright128` intrinsic in `secp256k1_u128_rshift` on MSVC
05873bb6b1 tweak_add: fix API doc for tweak=0
6ec3731e8c Simplify test PRNG implementation
fb5bfa4eed Add static test vector for Xoshiro256++
723e8ca8f7 Remove randomness tests
bc7c8db179 abi: Use dllexport for mingw builds
git-subtree-dir: src/secp256k1
git-subtree-split: c545fdc374964424683d9dac31a828adedabe860
c7db88af71 descriptor: assert we never parse a sane miniscript with no pubkey (Antoine Poinsot)
a49402a9ec qa: make sure we don't let unspendable Miniscript descriptors be imported (Antoine Poinsot)
639e3b6c97 descriptor: refuse to parse unspendable miniscript descriptors (Antoine Poinsot)
e3280eae1b miniscript: make GetStackSize() and GetOps() return optionals (Antoine Poinsot)
Pull request description:
`IsSane()` in Miniscript does not ensure a Script is actually spendable. This is an issue as we would accept any sane Miniscript when parsing a descriptor. Fix this by explicitly checking a Miniscript descriptor is both sane and spendable when parsing it.
This bug was exposed due to a check added in #22838 (https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226859880) that triggered a fuzz crash (https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1612510057).
ACKs for top commit:
sipa:
utACK c7db88af71
achow101:
ACK c7db88af71
Tree-SHA512: e79bc9f7842e98a4e8f358f05811fca51b15b4b80a171c0d2b17cf4bb1f578a18e4397bc2ece9817d392e0de0196ee6a054b7318441fd3566dd22e1f03eb64a5
4e5c933f6a Switch all callers from poly1305_auth to Poly1305 class (Pieter Wuille)
8871f7d1ae tests: add more Poly1305 test vectors (Pieter Wuille)
40e6c5b9fc crypto: add Poly1305 class with std::byte Span interface (Pieter Wuille)
50269b391f crypto: switch poly1305 to incremental implementation (Pieter Wuille)
Pull request description:
Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see #27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces:
* The additionally authenticated data (AAD), padded to 16 bytes.
* The ciphertext, padded to 16 bytes.
* The length of the AAD and the length of the ciphertext, together another 16 bytes.
Implementing RFC8439 using the existing `poly1305_auth` function requires creating a temporary copy with all these pieces of data concatenated just for the purpose of computing the tag (the approach used in #25361).
This PR replaces the poly1305 code with new code from https://github.com/floodyberry/poly1305-donna (with minor adjustments to make it match our coding style and use our utility functions, documented in the commit) which supports incremental operation, and then adds a C++ wrapper interface using std::byte Spans around it, and adds tests that incremental and all-at-once computation match.
ACKs for top commit:
achow101:
ACK 4e5c933f6a
theStack:
ACK 4e5c933f6a
stratospher:
tested ACK 4e5c933.
Tree-SHA512: df6e9a2a4a38a480f9e4360d3e3def5311673a727a4a85b008a084cf6843b260dc82cec7c73e1cecaaccbf10f3521a0ae7dba388b65d0b086770f7fbc5223e2a
fa6dfaaf45 scripted-diff: Use new FUZZ_TARGET macro everywhere (MarcoFalke)
fa36ad8b09 fuzz: Accept options in FUZZ_TARGET macro (MarcoFalke)
Pull request description:
The `FUZZ_TARGET` macros have many issues:
* The developer will have to pick the right macro to pass the wanted option.
* Adding a new option requires doubling the number of existing macros in the worst case.
Fix all issues by using only a single macro.
This refactor does not change behavior.
ACKs for top commit:
dergoegge:
ACK fa6dfaaf45
Tree-SHA512: 49a34553867a1734ce89e616b2d7c29b784a67cd8990db6573f0c7b18957636ef0c81d3d0d444a04c12cdc98bc4c4aa7a2ec94e6232dc363620a746e28416444
4da243ba02 qt: show own outputs on PSBT signing window (Hernan Marino)
Pull request description:
This fixes https://github.com/bitcoin-core/gui/issues/732 .
It allows you to identify your own addresses in the outputs of a transaction in the PSBT signing window. This enables easy identification of change outputs, and prevents certain attacks where someone (co-signers of a multisig, or others ) might trick you into signing a transaction while they are stealing the change, since prior to this modification there was no easy way of knowing this.
The identification of the output is similar to the way this is done in the transaction details window.
A sample output is :
ACKs for top commit:
achow101:
ACK 4da243ba02
jarolrod:
ACK 4da243ba02
Tree-SHA512: fa9901d2acc84472c11afcd0a59a859db598cdf5cea755b492178d3e7434b70d9bd8f554928938a2ff9920c8f397fef814ce14b416556c30fba0c3c1f62cd722
When simulating a snapshot, remove the HAVE_DATA status for blocks below the
snapshot height, to simulate never having downloaded them at all. This makes
tests more realistic (and more closely match what will happen when using
assumeutxo).
Block arrival information (and the preciousblock RPC, a related concept) are
both chainstate-agnostic, so these are moved to ChainstateManager. This should
just be a refactor, without any observable behavior changes.
When writing a new block to disk, if we have filled up the current block file,
then we flush and truncate that block file (to free allocated but unused
space) before advancing to the next one. When this happens, we have to
determine whether to also flush and truncate the corresponding undo file.
Undo data is only written when blocks are connected, not when blocks are
received. Thus it's possible that the corresponding undo file already has all
the data it will ever have, and we should flush/truncate it as we advance
files; or it's possible that there is more data we expect to write, and should
therefore defer flush/truncation until undo data is later written.
Prior to this commit, we made the determination of whether the undo file was
full of all requisite data by comparing against the chain tip. This patch
replaces that dependence on validation data structures by instead just tracking
the highest height of any block written in the undo file as we go.
31eca93a9e kernel: Remove StartShutdown calls from validation code (Ryan Ofsky)
Pull request description:
This change drops the last kernel dependency on shutdown.cpp. It also adds new hooks for libbitcoinkernel applications to be able to interrupt kernel operations when the chain tip changes.
This change is mostly a refactoring, but does slightly change `-stopatheight` behavior (see release note and commit message)
ACKs for top commit:
TheCharlatan:
ACK 31eca93a9e
furszy:
Concept and light review ACK 31eca93a
hebasto:
ACK 31eca93a9e, I have reviewed the code and it looks OK.
MarcoFalke:
lgtm ACK 31eca93a9e 🕷
Tree-SHA512: e26928436bcde658e842b1f92e9c24b1ce91031fb63b41aafccf3130bfff532b75338a269a2bb7558bff2973913f17b97a00fec3e7e0588e2ce44de097142047
35a2175ad8 fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()` (brunoerg)
Pull request description:
This PR adds fuzz coverage for `network` field in `Select()`, `Size()` and `GetAddr()`, there was only call to them without passing a network.
https://marcofalke.github.io/b-c-cov/fuzz.coverage/src/addrman.cpp.gcov.html
ACKs for top commit:
amitiuttarwar:
for the record, ACK 35a2175ad8 - only small changes from the version (previously) proposed in 27213
achow101:
ACK 35a2175ad8
mzumsande:
Code Review ACK 35a2175ad8, haven't tested this yet, but I will let the fuzzer run for a while now.
Tree-SHA512: dddb8322298d6c373c8e68d57538470b11825a9a310a355828c351d5c0b19ff6779d024a800e3ea90126d0c050e86f71fd22cd23d1a306c784cef0f82c45e3ca
e7cf8657e1 test: add unit test for local address advertising (Martin Zumsande)
f4754b9dfb net: restrict self-advertisements with privacy networks (Martin Zumsande)
e4d541c7cf net, refactor: pass reference for peer address in GetReachabilityFrom (Martin Zumsande)
62d73f5370 net, refactor: pass CNode instead of CNetAddr to GetLocalAddress (Martin Zumsande)
Pull request description:
The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from `CNetAddr::GetReachabilityFrom()`, self-advertise with the address that fits best to our peer.
It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement.
`GetReachabilityFrom()` currently only takes into account actual reachability, but not whether we'd _want_ to announce our identity for one network to peers from other networks, which is not straightforward in connection with privacy networks.
While the general approach is to prefer self-advertising with the address for the network our peer is on, there are several special situations in which we don't have one, and as a result could allow self-advertise other local addresses, for example:
A) We run i2p and clearnet, use `-i2pacceptincoming=0` (so we have no local i2p address), and we have a local ipv4 address. In this case, we'd advertise the ipv4 address to our outbound i2p peers.
B) Our `-discover` logic cannot detect any local clearnet addresses in our network environment, but we are actually reachable over clearnet. If we ran bitcoind clearnet-only, we'd always advertise the address our peer sees us with instead, and could get inbound peers this way. Now, if we also have an onion service running (but aren't using tor as a proxy for clearnet connections), we could advertise our onion address to clearnet peers, so that they would be able to connect our clearnet and onion identities.
This PR tries to avoid these situations by
1.) never advertising our local Tor or I2P address to peers from other networks.
2.) never advertising local addresses from non-anonymity networks to peers from Tor or I2P
Note that this affects only our own self-advertisements, the rules to forward other people's addrs are not changed.
[Edit] after Initial [discussion](https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1497176155): CJDNS is not being treated like Tor and I2P at least for now, because it has different privacy properties and for the practical reason that it has still very few bitcoin nodes.
ACKs for top commit:
achow101:
ACK e7cf8657e1
vasild:
ACK e7cf8657e1
luke-jr:
utACK e7cf8657e1
Tree-SHA512: 3db8415dea6f82223d11a23bd6cbb3b8cf68831321280e926034a1f110cbe22562570013925f6fa20d8f08e41d0202fd69c733d9f16217318a660d2a1a21b795
New code can call the method without having first to retrieve the raw
FILE* pointer via Get().
Also, move implementation to the cpp file. Can be reviewed with:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
* Add m_ prefix to the std::FILE member variable
* Add std namespace where possible, to avoid confusion with member
functions of the same name.
* Add AutoFile::feof() member function, to be used in place of
std::feof(AutoFile::Get())
* Simplify fclose() in terms of release()
* Fix typo in the error message in the ignore member function.
This also removes the old poly1305_auth interface, as it no longer serves any
function. The new Poly1305 class based interface is more modern and safe.
This code is taken from poly1305-donna-32.h, poly1305-donna.h, poly1305-donna.c
from https://github.com/floodyberry/poly1305-donna, commit
e6ad6e091d30d7f4ec2d4f978be1fcfcbce72781, with the following modifications:
* Coding style (braces around one-line indented if/for loops).
* Rename unsigned long (long) to uint32_t and uint64_t.
* Rename poly1305_block_size to POLY1305_BLOCK_SIZE.
* Adding noexcept to functions.
* Merging poly1305_state_internal_t and poly1305_context types.
* Merging code from multiple files.
* Place all imported code in the poly1305_donna namespace.
0bf87476f5 test: add ChaCha20 test triggering 32-bit block counter overflow (Sebastian Falbesoner)
7f2a985147 tests: improve ChaCha20 unit tests (Pieter Wuille)
511a8d406e crypto: Implement RFC8439-compatible variant of ChaCha20 (Pieter Wuille)
Pull request description:
Based on and replaces part of #25361, part of the BIP324 project (#27634). See also #19225 for background.
There are two variants of ChaCha20 in use. The currently implemented one uses a 64-bit nonce and a 64-bit block counter, while the one used in RFC8439 (and thus BIP324) uses a 96-bit nonce and 32-bit block counter. This PR changes the logic to use the 96-bit nonce variant, though in a way that's compatible with >256 GiB output (by automatically incrementing the first 32-bit part of the nonce when the block counter overflows).
For those who reviewed the original PR, the biggest change is here that the 96-bit nonce is passed as a Nonce96 type (pair of 32-bit + 64-bit integer) rather than a 12-byte array.
ACKs for top commit:
achow101:
ACK 0bf87476f5
theStack:
Code-review ACK 0bf87476f5
Tree-SHA512: 62e4cbd5388b8d50ef1a0dc99b6f4ad36c7b4419032035f8e622dda63a62311dd923032217e20054bcd836865d4be5c074f9e5538ca158f94f08eab75c5519c1
The shift operators will call the write or read member function, which
already does the check. Also, call sites are free to directly call
::(Un)Serialize(s, obj) to skip this check, so removing it increases
consistency.
With C++11 (and later), the duplicate variable is no longer needed.
Also, run clang-format on the namespace, as the script in the next
commit relies on a specific format. This prevents a clang-format run in
the future from breaking the script. (Review hint: --ignore-all-space).
This change drops the last kernel dependency on shutdown.cpp. It also adds new
hooks for libbitcoinkernel applications to be able to interrupt kernel
operations when the chain tip changes.
This is a refactoring that does not affect behavior. (Looking at the code it
can appear like the new break statement in the ActivateBestChain function is a
change in behavior, but actually the previous StartShutdown call was indirectly
triggering a break before, because it was causing m_chainman.m_interrupt to be
true. The new code just makes the break more obvious.)
89ba8905f5 test: indexes, fix on error infinite loop (furszy)
Pull request description:
Coming from https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703, I thought that we were going to fix it there but seems that got merged without it for some reason.
As index sync failures trigger a shutdown request without notifying `BaseIndex::BlockUntilSyncedToCurrentChain` in any way, we also need to check whether a shutdown was requested or not inside 'IndexWaitSynced'.
Otherwise, any error inside the index sync process will hang the test forever.
ACKs for top commit:
MarcoFalke:
lgtm ACK 89ba8905f5
jamesob:
ACK 89ba890
ryanofsky:
Code review ACK 89ba8905f5. Just comment update since last review
Tree-SHA512: 1f6daf34e51d3fbc802799bfa4ac0ef0d8f774db5f9e2f5d35df18a77679778475c94efc3da1fb723ebaf3583e4075e4a5cbe4a5104ad0c50e2b32076e247b29
The `UNKNOWN_DESCRIPTOR` error comes from the
`WalletDescriptor::DeserializeDescriptor` std::ios_base
exception, which contains further information about the
parsing error.
This has the benefit of moving the StartShutdown call out of the
blockstorage file and thus out of the kernel's responsibility. The user
can now decide if he wants to start shutdown / interrupt after a block
import or not.
As index sync failures trigger a shutdown request without notifying
BaseIndex::BlockUntilSyncedToCurrentChain in any way, we also need
to check whether a shutdown was requested or not inside 'IndexWaitSynced'.
Otherwise, any error inside the index sync process will hang the test
forever.
ca91c244ef index: verify blocks data existence only once (furszy)
fcbdaeef4d init: don't start indexes sync thread prematurely (furszy)
2ec89f1970 refactor: simplify pruning violation check (furszy)
c82ef91eae make GetFirstStoredBlock assert that 'start_block' always has data (furszy)
430e7027a1 refactor: index, decouple 'Init' from 'Start' (furszy)
225e213110 refactor: init indexes, decouple 'Start()' from the creation step (furszy)
2ebc7e68cc doc: describe 'init load' thread actions (Martin Zumsande)
04575106b2 scripted-diff: rename 'loadblk' thread name to 'initload' (furszy)
ed4462cc78 init: start indexes sync earlier (furszy)
Pull request description:
Simplifies index startup code, eliminating the `g_indexes_ready_to_sync` variable,
deduplicating code and moving the prune violation check out of the `BaseIndex` class.
Also makes startup more efficient by running the prune violation check once for all indexes
instead of once for each index, and by delaying the prune violation check and moving it off
of the main thread so the node can start up faster and perform the block data availability
verification even when the '-reindex" or the "-reindex-chainstate" flags are enabled (which
hasn't being possible so far).
ACKs for top commit:
ryanofsky:
Code review ACK ca91c244ef. Just rebase and suggested changes since last review (Start return check, and code simplification)
TheCharlatan:
re-ACK ca91c244ef
Tree-SHA512: e9c98ce89aeb29e8d0f505f17b34aa54fe44efefbf017f4746e3b446ab4de25ade4f707254a0bbe4b99b69731b04a4067ce529eb7aa834ced196784b694cf7ce
At present, during init, we traverse the chain (once per index)
to confirm that all necessary blocks to sync each index up to
the current tip are present.
To make the process more efficient, we can fetch the oldest block
from the indexers and perform the chain data existence check from
that point only once.
This also moves the pruning violation check to the end of the
'loadinit' thread, which is where the reindex, block loading and
chain activation processes happen.
Making the node's startup process faster, allowing us to remove
the global g_indexes_ready_to_sync flag, and enabling the
execution of the pruning violation verification even when the
reindex or reindex-chainstate flags are enabled (which has being
skipped so far).
By moving the 'StartIndexes()' call into the 'initload'
thread, we can remove the threads active wait. Optimizing
the available resources.
The only difference with the current state is that now the
indexes threads will only be started when they can process
work and not before it.
By generalizing 'GetFirstStoredBlock' and implementing
'CheckBlockDataAvailability' we can dedup code and
avoid repeating work when multiple indexes are enabled.
E.g. get the oldest block across all indexes and
perform the pruning violation check from that point
up to the tip only once (this feature is being introduced
in a follow-up commit).
This commit shouldn't change behavior in any way.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
