c0bf667912 index: add [nodiscard] attribute to functions writing to the db (furszy)
eef595560e index: coinstats reorg, fail when block cannot be reversed (furszy)
Pull request description:
Found it while reviewing https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1310863359.
During a reorg, continuing execution when a block cannot be reversed leaves the
coinstats index in an inconsistent state.
This was surely overlooked when 'CustomRewind' was implemented.
ACKs for top commit:
ryanofsky:
Code review ACK c0bf667912. Only change since last review is new commit adding [[nodiscard]]
Tree-SHA512: f4fc8522508d23e4fff09a29c935971819b1bd3b2a260e08e2e2b72f9340980d74fbec742a58fe216baf61d27de057c7c8300e8fa075f8507cd1227f128af909
Before this commit the V2Transport::m_send_buffer is used to store the
garbage:
* During MAYBE_V1 state, it's there despite not being sent.
* During AWAITING_KEY state, while it is being sent.
* At the end of the AWAITING_KEY state it cannot be wiped as it's still
needed to compute the garbage authentication packet.
Change this by introducing a separate m_send_garbage field, taking over
the first and last role listed above. This means the garbage is only in
the send buffer when it's actually being sent, removing a few special
cases related to this.
This removes the ability for BIP324Cipher to generate its own key, moving that
responsibility to the caller (mostly, V2Transport). This allows us to write
the random-key V2Transport constructor by delegating to the explicit-key one.
9f55773a37 test: refactor: usdt_mempool: store all events (stickies-v)
bc43270450 test: refactor: remove unnecessary nonlocal (stickies-v)
326db63a68 test: log sanity check assertion failures (stickies-v)
f5525ad680 test: store utxocache events (stickies-v)
f1b99ac94f test: refactor: deduplicate handle_utxocache_* logic (stickies-v)
ad90ba36bd test: refactor: rename inbound to is_inbound (stickies-v)
afc0224cdb test: refactor: remove unnecessary blocks_checked counter (stickies-v)
Pull request description:
Various cleanups to the USDT functional tests, largely (but not exclusively) follow-ups to https://github.com/bitcoin/bitcoin/pull/27831#pullrequestreview-1491438045. Except for slightly different logging behaviour in "test: store utxocache events" and "test: log sanity check assertion failures", this is a refactor PR, removing unnecessary code and (imo) making it more readable and maintainable.
The rationale for each change is in the corresponding commit message.
Note: except for "test: store utxocache events" (which relies on its parent, and I separated into two commits because we may want the parent but not the child), all commits are stand-alone and I'm okay with dropping one/multiple commits if they turn out to be controversial or undesired.
ACKs for top commit:
0xB10C:
ACK 9f55773a37. Reviewed the code and ran the USDT interface tests. I stepped through the commits and think all changes are reasonable.
Tree-SHA512: 6c37a0265b6c26d4f9552a056a690b8f86f7304bd33b4419febd8b17369cf6af799cb87c16df35d0c2a1b839ad31de24661d4384eafa88816c2051c522fd3bf5
8d6228fc1f consensus/validation.h: remove needless GetTransactionOutputWeight helper (Antoine Poinsot)
Pull request description:
Introduced in #26567. My bad. Thanks AJ for noticing.
ACKs for top commit:
ajtowns:
utACK 8d6228fc1f
Tree-SHA512: cf13647b4aac82fb6a54ae0338e3928e9bdf226ed4f5e91d529996328471744132db2bee9676e0b3f40a8bbe0e0ca51a9e5f91560a84e0f33597290551a1ee18
e73d2a8018 refactor: remove clientversion include from dbwrapper.h (Cory Fields)
4240a082b8 refactor: Use DataStream now that version/type are unused (Cory Fields)
f15f790618 Remove version/hashing options from CBlockLocator/CDiskBlockIndex (Cory Fields)
Pull request description:
This is also a much simpler replacement for #28327.
There are version fields in `CBlockLocator` and `CDiskBlockIndex` that have always been written but discarded when read.
I intended to convert them to use SerParams as introduced by #25284, which [ended up looking like this](3e3af45165). However because we don't currently have any definition of what a hash value would mean for either one of those, and we've never assigned the version field any meaning, I think it's better to just not worry about them.
If we ever need to assign meaning in the future, we can introduce `SerParams` as was done for `CAddress`.
As for the dummy values chosen:
`CDiskBlockIndex::DUMMY_VERSION` was easy as the highest ever client version, and I don't expect any objection there.
`CBlockLocator::DUMMY_VERSION` is hard-coded to the higest _PROTOCOL_ version ever used. This is to avoid a sudden bump that would be visible on the network if CLIENT_VERSION were used instead. In the future, if we ever need to use the value, we can discard anything in the CLIENT_VERSION range (for a few years as needed), as it's quite a bit higher.
While reviewing, I suggest looking at the throwaway `SerParams` commit above as it shows where the call-sites are. I believe that should be enough to convince one's self that hashing is never used.
ACKs for top commit:
TheCharlatan:
Re-ACK e73d2a8018
ajtowns:
reACK e73d2a8018
Tree-SHA512: 45b0dd7c2e918493e2ee92a8e35320ad17991cb8908cb811150a96c5fd584ce177c775baeeb8675a602c90b9ba9203b8cefc0a2a0c6a71078b1d9c2b41e1f3ba
db9888feec net: detect wrong-network V1 talking to V2Transport (Pieter Wuille)
91e1ef8684 test: add unit tests for V2Transport (Pieter Wuille)
297c888997 net: make V2Transport preallocate receive buffer space (Pieter Wuille)
3ffa5fb49e net: make V2Transport send uniformly random number garbage bytes (Pieter Wuille)
0be752d9f8 net: add short message encoding/decoding support to V2Transport (Pieter Wuille)
8da8642062 net: make V2Transport auto-detect incoming V1 and fall back to it (Pieter Wuille)
13a7f01557 net: add V2Transport class with subset of BIP324 functionality (Pieter Wuille)
dc2d7eb810 crypto: Spanify EllSwiftPubKey constructor (Pieter Wuille)
5f4b2c6d79 net: remove unused Transport::SetReceiveVersion (Pieter Wuille)
c3fad1f29d net: add have_next_message argument to Transport::GetBytesToSend() (Pieter Wuille)
Pull request description:
This is part of #27634.
This implements the BIP324 v2 transport (which implements all of what the BIP calls transport layer *and* application layer), though in a non-exposed way. It is tested through an extensive fuzz test, which verifies that v2 transports can talk to v2 transports, and v1 transports can talk to v2 transports, and a unit test that exercises a number of unusual scenarios. The transport is functionally complete, including:
* Autodetection of incoming V1 connections.
* Garbage, both sending and receiving.
* Short message type IDs, both sending and receiving.
* Ignore packets (receiving only, but tested in a unit test).
* Session IDs are visible in `getpeerinfo` output (for manual comparison).
Things that are not included, left for future PRs, are:
* Actually using the v2 transport for connections.
* Support for the `NODE_P2P_V2` service flag.
* Retrying downgrade to V1 when attempted outbound V2 connections immediately fail.
* P2P functional and unit tests
ACKs for top commit:
naumenkogs:
ACK db9888feec
theStack:
re-ACK db9888feec
mzumsande:
Code Review ACK db9888feec
Tree-SHA512: 8906ac1e733a99e1f31c9111055611f706d80bbfc2edf6a07fa6e47b21bb65baacd1ff17993cbbf588063b2f5ad30b3af674a50c7bc8e8ebf4671483a21bbfeb
1580e3be83 fuzz: add ConstructPubKeyBytes function (josibake)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/28246 and https://github.com/bitcoin/bitcoin/pull/28122 , we add a `PubKeyDestination` and a `V0SilentPaymentsDestination`. Both of these PRs update `fuzz/util.cpp` and need a way to create well-formed pubkeys. Currently in `fuzz/util.cpp`, we have some logic for creating pubkeys in the multisig data provider. This logic is duplicated in #28246 and duplicated again in #28122. Seems much better to have a `ConstructPubKeyBytes` function that both PRs (and any future work) can reuse.
This PR introduces a function to do this and has the existing code use it. While the purpose is to introduce a utility function, the previous multisig code used `ConsumeIntegralInRange(4, 7)` which would have created some uncompressed pubkeys with the prefix 0x05, which is incorrect (see https://bitcoin.stackexchange.com/questions/57855/c-secp256k1-what-do-prefixes-0x06-and-0x07-in-an-uncompressed-public-key-signif)
tldr; using `PickValueFromArray` is more correct as it limits to the set of defined prefixes for compressed and uncompressed pubkeys.
ACKs for top commit:
Sjors:
ACK 1580e3be83
Tree-SHA512: c87c8bcd1f6b3a97ef772be93102efb912811c59f32211cfd531a116f1da8a57c8c6ff106b34f2a2b88d8b34fb5bc30d9f9ed6d2720113ffcaaa2f8d5dc9eb27
During a reorg, continuing execution when a block cannot be
reversed leaves the coinstats index in an inconsistent state,
which was surely overlooked when 'CustomRewind' was implemented.
This introduces a V2Transport with a basic subset of BIP324 functionality:
* no ability to send garbage (but receiving is supported)
* no ability to send decoy packets (but receiving them is supported)
* no support for short message id encoding (neither encoding or decoding)
* no waiting until 12 non-V1 bytes have been received
* (and thus) no detection of V1 connections on the responder side
(on the sender side, detecting V1 is not supported either, but that needs
to be dealt with at a higher layer, by reconnecting)
Before this commit, there are only two possibly outcomes for the "more" prediction
in Transport::GetBytesToSend():
* true: the transport itself has more to send, so the answer is certainly yes.
* false: the transport has nothing further to send, but if vSendMsg has more message(s)
left, that still will result in more wire bytes after the next
SetMessageToSend().
For the BIP324 v2 transport, there will arguably be a third state:
* definitely not: the transport has nothing further to send, but even if vSendMsg has
more messages left, they can't be sent (right now). This happens
before the handshake is complete.
To implement this, we move the entire decision logic to the Transport, by adding a
boolean to GetBytesToSend(), called have_next_message, which informs the transport
whether more messages are available. The return values are still true and false, but
they mean "definitely yes" and "definitely no", rather than "yes" and "maybe".
fa626af3ed Remove unused legacy CHashVerifier (MarcoFalke)
fafa3fc5a6 test: add tests that exercise WithParams() (MarcoFalke)
fac81affb5 Use serialization parameters for CAddress serialization (MarcoFalke)
faec591d64 Support for serialization parameters (MarcoFalke)
fac42e9d35 Rename CSerAction* to Action* (MarcoFalke)
aaaa3fa947 Replace READWRITEAS macro with AsBase wrapping function (MarcoFalke)
Pull request description:
It seems confusing that picking a wrong value for `ADDRV2_FORMAT` could have effects on consensus. (See the docstring of `ADDRV2_FORMAT`).
Fix this by implementing https://github.com/bitcoin/bitcoin/issues/19477#issuecomment-1147421608 .
This may also help with libbitcoinkernel, see https://github.com/bitcoin/bitcoin/pull/28327
ACKs for top commit:
TheCharlatan:
ACK fa626af3ed
ajtowns:
ACK fa626af3ed
Tree-SHA512: 229d379da27308890de212b1fd2b85dac13f3f768413cb56a4b0c2da708f28344d04356ffd75bfcbaa4cabf0b6cc363c4f812a8f1648cff9e436811498278318
583af18fd1 fuzz: introduce and use `ConsumePrivateKey` helper (Sebastian Falbesoner)
Pull request description:
In the course of reviewing BIP324 related PRs I noticed a frequent pattern of creating private keys (`CKey` instances) with data consumed from the fuzz data provider:
```
auto key_data = provider.ConsumeBytes<unsigned char>(32);
key_data.resize(32);
CKey key;
key.Set(key_data.begin(), key_data.end(), /*fCompressedIn=*/true);
```
This PR introduces a corresponding helper `ConsumePrivateKey` in order to deduplicate code. The compressed flag can either be set to a fixed value, or, if `std::nullopt` is passed (=default), is also consumed from the fuzz data provider via `.ConsumeBool()`.
Note that this is not a pure refactor, as some of the replaced call-sites previously consumed a random length (`ConsumeRandomLengthByteVector`) instead of a fixed size of 32 bytes for key data. As far as I can see, there is not much value in using a random size, as in all those cases we can only proceed or do something useful with a valid private key, and key data with sizes other than 32 bytes always lead to invalid keys.
ACKs for top commit:
sipa:
utACK 583af18fd1
brunoerg:
crACK 583af18fd1
Tree-SHA512: 58a178432ba1eb0a2f7597b6700e96477e8b10f429ef642445a52db12e74d81aec307888315b772bfda9610f90df3e1d556cf024c2bef1d520303b12584feaaa
10546a569c wallet: accurately account for the size of the witness stack (Antoine Poinsot)
9b7ec393b8 wallet: use descriptor satisfaction size to estimate inputs size (Antoine Poinsot)
8d870a9873 script/signingprovider: introduce a MultiSigningProvider (Antoine Poinsot)
fa7c46b503 descriptor: introduce a method to get the satisfaction size (Antoine Poinsot)
bdba7667d2 miniscript: introduce a helper to get the maximum witness size (Antoine Poinsot)
4ab382c2cd miniscript: make GetStackSize independent of P2WSH context (Antoine Poinsot)
Pull request description:
The wallet currently estimates the size of a signed input by doing a dry run of the signing logic. This is unnecessary since all outputs we can sign for can be represented by a descriptor, and we can derive the size of a satisfaction ("signature") directly from the descriptor itself.
In addition, the current approach does not generalize well: dry runs of the signing logic are only possible for the most basic scripts. See for instance the discussion in #24149 around that.
This introduces a method to get the maximum size of a satisfaction from a descriptor, and makes the wallet use that instead of the dry-run.
ACKs for top commit:
sipa:
utACK 10546a569c
achow101:
re-ACK 10546a569c
Tree-SHA512: 43ed1529fbd30af709d903c8c5063235e8c6a03b500bc8f144273d6184e23a53edf0fea9ef898ed57d8a40d73208b5d935cc73b94a24fad3ad3c63b3b2027174
fae0b21e6c test: Combine sync_send_with_ping and sync_with_ping (MarcoFalke)
Pull request description:
This reduces bloat, complexity, and makes tests less fragile to intermittent failures, see https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1315648343.
This should not cause any noticeable slowdown, or may even be faster, because active polling will be done at most once.
ACKs for top commit:
glozow:
Concept ACK fae0b21e6c
theStack:
ACK fae0b21e6c🏓
Tree-SHA512: 6c543241a7b85458dc7ff6a6203316b80a6227d83d38427e74f53f4c666a882647a8a221e5259071ee7bb5dfd63476fb03c9b558a1ea546734b14728c3c619ba
96f2cf8d2c build: use -muse-unaligned-vector-move for Windows (fanquake)
Pull request description:
We currently work around a longstanding GCC issue with aligned vector instructions, by patching the behaviour we want into GCC (see discussion in #24736). Possibly in response to the GCC thread (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=54412#c40), a new option was [introduced into the binutils assembler](https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=c8480b58e1968f209b6365af7422678f348222c2) with the 2.38 release:
```
x86: Add -muse-unaligned-vector-move to assembler
Unaligned load/store instructions on aligned memory or register are as
fast as aligned load/store instructions on modern Intel processors. Add
a command-line option, -muse-unaligned-vector-move, to x86 assembler to
encode encode aligned vector load/store instructions as unaligned
vector load/store instructions.
```
ACKs for top commit:
theuni:
ACK 96f2cf8d2c.
Tree-SHA512: f5aaa125922bb17bbb51454103b3394d293184214b0dea554c36c2f130488a3ede2f39678044ec846fa0fdf4cd441d4227f4565c29d380f5a73b50bf6f3b9a67
fae405556d scripted-diff: Rename CBlockTreeDB -> BlockTreeDB (MarcoFalke)
faf63039cc Fixup style of moved code (MarcoFalke)
fa65111b99 move-only: Move CBlockTreeDB to node/blockstorage (MarcoFalke)
fa8685597e index: Drop legacy -txindex check (MarcoFalke)
fa69148a0a scripted-diff: Use blocks_path where possible (MarcoFalke)
Pull request description:
The only reason for the check was to print a warning about an increase in storage use. Now that 22.x is EOL and everyone should have migrated (or decided to not care about storage use), remove the check.
Also, a move-only commit is included. (Rebased from https://github.com/bitcoin/bitcoin/pull/22242)
ACKs for top commit:
TheCharlatan:
ACK fae405556d, though I lack historical context to really judge the second commit fa8685597e.
stickies-v:
ACK fae405556d
Tree-SHA512: 9da8f48767ae52d8e8e21c09a40c949cc0838794f1856cc5f58a91acd3f00a3bca818c8082242b3fdc9ca5badb09059570bb3870850d3807b75a8e23b5222da1
c0da4f60e2 Squashed 'src/secp256k1/' changes from c545fdc374..199d27cea3 (Pieter Wuille)
Pull request description:
We had previously pulled in a non-released commit along with #27479. The necessary changes have now been released in version 0.4.0, so update to that.
ACKs for top commit:
hebasto:
ACK 0e0fc18c3c, having a zero diff with my local branch that updates the `secp256k1` subtree up to v0.4.0.
fanquake:
ACK 0e0fc18c3c
Tree-SHA512: 8b771e7da89b9cdb7a680b9dd4eb99a6f737b32914b0b62c485b3c484e5438f9f60942030d3072243aaa196da22d2b1fdb3b6a668d75a46e6ac78c9d86b4bd8b
fa07ac48d8 ci: Asan with -ftrivial-auto-var-init=pattern (MarcoFalke)
Pull request description:
This makes memory bugs deterministic. `-ftrivial-auto-var-init=pattern` is incompatible with other memory sanitizers (like valgrind and msan), but that is irrelevant here, because the address sanitizer in this fuzz CI config is already incompatible with them.
`-ftrivial-auto-var-init=pattern` goes well with `-fsanitize=bool` and `-fsanitize=enum`, but those are already enabled via `-fsanitize=undefined`. See https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#available-checks
ACKs for top commit:
fanquake:
ACK fa07ac48d8 - going to get back to fixing up the cxxflags usage in CI, but not a blocker here:
Tree-SHA512: 2ea6c5128a9cd262bdd1b1475c7e1be23ce2c520fad05f6c2aace6c29e203573323c0564d272e25da35ad5ff46fde488a5eae1ed14d3349e793d14a5e2533fb1
fbcacd4cf0 test: remove fixed timeouts from feature_config_args (Martin Zumsande)
Pull request description:
Fixes#28290
These fixed timeouts aren't affected by the `timeout_factor` option and can therefore cause timeouts in slow environments.
They are also unnecessary for the test because they measure the wrong thing:
While there is an internal waiting time of 60s within `ThreadOpenConnections` (beginning only when that thread is started) for fixed seeds querying, the timeouts here don't measure that but the time from startup until a debug log message is encountered, during which many other things happen in init, so they don't make much sense to me in the first place.
ACKs for top commit:
MarcoFalke:
lgtm ACK fbcacd4cf0
Tree-SHA512: 7bb3b7db2f9666b1929ffb7773c838ee98b0845569428e5d00ecf5234973d534c4f474e213896c71baabd6096a79347bd21b41a17b130053049714eb8a447c79
668aa6af8d test: p2p: check that `getaddr` msgs are only responded once per connection (Sebastian Falbesoner)
Pull request description:
This simple PR adds missing test coverage for ignoring repeated `getaddr` requests (introduced in #7856, commit 66b07247a7):
6f03c45f6b/src/net_processing.cpp (L4642-L4648)
ACKs for top commit:
MarcoFalke:
lgtm ACK 668aa6af8d
brunoerg:
crACK 668aa6af8d
Tree-SHA512: edcdc6501c684fb41911e393f55ded9b044cd2f92918877eca152edd5a4287d1a9d57ae999f1cb42185eae00c3a0af411fcb9bcd5b990ef48849c3834b141584
