This was only explicitly used in the tests, where it can be replaced by
wrapping the original raw file pointer into a CAutoFile on creation and
then calling CAutoFile::fclose().
Also, it was used in LoadExternalBlockFile(), where it can also be
replaced by the (implicit call to the) CAutoFile destructor after
wrapping the original raw file pointer in a CAutoFile.
508d05f8a7b511dd53f543df8899813487eb03e5 [fuzz] Don't use afl++ deferred forkserver mode (dergoegge)
Pull request description:
Fixes#28469
This makes our afl++ harness essentially behave like libFuzzer, with the exception that the whole program does fully reset every 100000 iterations. 100000 is somewhat arbitrary and we could also go with `std::numeric_limits<unsigned in>::max()` but a smaller limit does allow for the occasional reset to counter act some amount of instability in the fuzzing loop (e.g. non-determinism, statefulness).
It's a bit of a shame to do this just for the targets whose initial state can't be forked (e.g. threads) because other targets do benefit from not having to redo the state setup. An alternative would be https://github.com/bitcoin/bitcoin/issues/28469#issuecomment-1717526774:
```
If the goal is to be maximally performant, the fork would need to happen for each fuzz target specifically.
I guess it can be achieved by wrapping __AFL_INIT(); into a helper function and then require all fuzz
target initialize() to call it?
```
ACKs for top commit:
MarcoFalke:
lgtm ACK 508d05f8a7b511dd53f543df8899813487eb03e5
Tree-SHA512: d9fe94e2e3198795f8fb58f67eb383531a534bcd4ec75a1f0ae6ccb5531863dbc09800bb7d77536417745c4c8bc49a4f84dcc959918b27d4997a270eeacb0e7e
3fcd7fc7ff563bdc0e2bba66b4cbe72d898c876e Do not use std::vector = {} to release memory (Pieter Wuille)
Pull request description:
It appears that invoking `v = {};` for an `std::vector<...> v` is equivalent to `v.clear()`, which does not release its allocated memory. There are a number of places in the codebase where it appears to be used for that purpose however (mostly written by me). Replace those with `std::vector<...>{}.swap(v);` (using a helper function `ClearShrink` in util/vector.h).
To explain what is going on: `v = {...};` is equivalent in general to `v.operator=({...});`. For many types, the `{}` is converted to the type of `v`, and then assigned to `v` - which for `std::vector` would ordinarily have the effect of clearing its memory (constructing a new empty vector, and then move-assigning it to `v`). However, since `std::vector<T>` has an `operator=(std::initializer_list<T>)` defined, it has precedence (since no implicit conversion is needed), and with an empty list, that is equivalent to `clear()`.
I did consider using `v = std::vector<T>{};` as replacement for `v = {};` instances where memory releasing is desired, but it appears that it does not actually work universally either. `V{}.swap(v);` does.
ACKs for top commit:
ajtowns:
utACK 3fcd7fc7ff563bdc0e2bba66b4cbe72d898c876e
stickies-v:
ACK 3fcd7fc7ff563bdc0e2bba66b4cbe72d898c876e
theStack:
Code-review ACK 3fcd7fc7ff563bdc0e2bba66b4cbe72d898c876e
Tree-SHA512: 6148558126ec3c8cfd6daee167ec1c67b360cf1dff2cbc132bd71768337cf9bc4dda3e5a9cf7da4f7457d2123288eeba77dd78f3a17fa2cfd9c6758262950cc5
f18f9ef4d31c70e2d71ab90a24511692821418c3 Amend bumpfee for inputs with overlapping ancestry (Murch)
2e35e944dab09eff30952233f8dfc0b12c4553d5 Bump unconfirmed parent txs to target feerate (Murch)
3e3e05241128f68cf12f73ee06ff997395643885 coinselection: Move GetSelectionWaste into SelectionResult (Andrew Chow)
c57889da6650715f3e1153b6104bbdae15fcac90 [node] interface to get bump fees (glozow)
c24851be945b2a633ee44ed3c8a501eee5580b62 Make MiniMinerMempoolEntry fields private (Murch)
ac6030e4d8f7d578cd4a8593f41189efca548064 Remove unused imports (Murch)
d2f90c31ef3b8dee5a3e0804ecc62fa1cfec7cd5 Fix calculation of ancestor set feerates in test (Murch)
a1f7d986e0211e54e21a1d4a570e5f15294dca72 Match tx names to index in miniminer overlap test (Murch)
Pull request description:
Includes some commits to address follow-ups from #27021: https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1554675156
Reduces the effective value of unconfirmed UTXOs by the fees necessary to bump their ancestor transactions to the same feerate.
While the individual UTXOs always account for their full ancestry before coin-selection, we can correct potential overestimates with a second pass where we establish the ancestry and bump fee for the whole input set collectively.
Fixes#9645Fixes#9864Fixes#15553
ACKs for top commit:
S3RK:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
ismaelsadeeq:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
achow101:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
brunoerg:
crACK f18f9ef4d31c70e2d71ab90a24511692821418c3
t-bast:
ACK f18f9ef4d3, I reviewed the latest changes and run e2e tests against eclair, everything looks good 👍
Tree-SHA512: b65180c4243b1f9d13c311ada7a1c9f2f055d530d6c533b78c2068b50b8c29ac1321e89e85675b15515760d4f1b653ebd9da77b37c7be52d9bc565a3538f0aa6
de8f9123afbecc3b4f59fa80af8148bc865d0588 test: cover read-only blockstore (Matthew Zipkin)
5c2185b3b624ce87320ec16412f98ab591a5860c ci: enable chattr +i capability inside containers (Matthew Zipkin)
e573f2420244c583e218f51cd0d3a3cac6731003 unit test: add coverage for BlockManager (Matthew Zipkin)
Pull request description:
This PR adds unit and functional tests to cover the behavior described in #2039. In particular, that bitcoind will crash on startup if a reindex is requested but the `blk` files are read-only. Eventually this behavior can be updated with https://github.com/bitcoin/bitcoin/pull/27039. This PR just commits the test coverage from #27039 as suggested in https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1584915782
ACKs for top commit:
jonatack:
ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 modulo suggestions in https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1319010039, tested on macOS, but not on Linux for the Linux-related change in the last push
achow101:
ACK de8f9123afbecc3b4f59fa80af8148bc865d0588
MarcoFalke:
lgtm ACK de8f9123afbecc3b4f59fa80af8148bc865d0588 📶
Tree-SHA512: b9bd684035dcea11c901b649fc39f397a2155a9a8459f3348e67947e387e45312fddeccb52981aef486f8a31deebb5356a7901c1bb94b78f82c24192a369af73
Deferring the forkserver initialization doesn't make sense for some of
our targets since they involve state that can't be forked (e.g.
threads). We therefore remove the use of __AFL_INIT entirely.
We also increase the __AFL_LOOP count to 100000. Our fuzz targets are
meant to all be deterministic and stateless therefore this should be
fine.
97e2e1d641016cd7b74848b9560e3771f092c1ea [fuzz] Use afl++ shared-memory fuzzing (dergoegge)
Pull request description:
Using shared-memory is faster than reading from stdin, see 7d2122e059/instrumentation/README.persistent_mode.md
ACKs for top commit:
MarcoFalke:
review ACK 97e2e1d641016cd7b74848b9560e3771f092c1ea
Tree-SHA512: 7e71b5f84835e41531c19ee959be2426da245869757de8e5dd1c730ae83ead650e2ef75f4d594d7965f661821a4ffbd27be84d3ce623702991501b34a8d02fc3
d5067651991f3e6daf456ba13c7036ddc4545352 [refactor] Remove compat.h from kernel headers (TheCharlatan)
36193af47c8dcff53e59498c416b85b59e0d0f91 [refactor] Remove netaddress.h from kernel headers (TheCharlatan)
2b08c55f01996e0b05763f05eac50b83ba9d5a8e [refactor] Add CChainParams member to CConnman (TheCharlatan)
f0d1d8b35c3aa9f2f923f74e3dbbf1e5ece4cd2f [refactor] Add missing includes for next commit (TheCharlatan)
534b314a7401d44f51aabd4565f97be9ee411740 kernel: Move MessageStartChars to its own file (TheCharlatan)
9be330b654cfbd792620295f3867f592059d6a7a [refactor] Define MessageStartChars as std::array (TheCharlatan)
37e2b011136ca1cf00dfb9e575d12f0d035a6a2c [refactor] Allow std::array<std::byte, N> in serialize.h (MarcoFalke)
Pull request description:
This removes the non-consensus critical `protocol.h` and `netaddress.h` headers from the kernel headers. With this patch, they are no longer required to include in order to use the libbitcoinkernel library. This also allows for the removal of the `compat.h` header from the kernel headers.
As an added future benefit it also reduces the number of of kernel headers that include the platform specific `bitcoin-config.h`.
For those interested, the currently required kernel headers can be inspected visually with the [sourcetrail](https://github.com/CoatiSoftware/Sourcetrail) tool by looking at the required includes of `bitcoin-chainstate.cpp`.
---
This is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587), namely its stage 1 step 3: Decouple most non-consensus headers from libbitcoinkernel.
ACKs for top commit:
stickies-v:
re-ACK d506765
hebasto:
ACK d5067651991f3e6daf456ba13c7036ddc4545352.
ajtowns:
utACK d5067651991f3e6daf456ba13c7036ddc4545352
MarcoFalke:
lgtm ACK d5067651991f3e6daf456ba13c7036ddc4545352 🍛
Tree-SHA512: 6f90ea510a302c2927e84d16900e89997c39b8ff3ce9d4effeb8a134bd29cc52bd9e81e51aaa11f7496bad00025b78a58b88c5a9e0bb3f4ebbe9a76309215fb7
fa19c914f7fe7be127c0fb330b41ff7c091f40aa scripted-diff: Rename CBufferedFile to BufferedFile (MarcoFalke)
fa2f2413b87f5fc1e5c92bf510beebdcd0091714 Remove unused GetType() from CBufferedFile and CAutoFile (MarcoFalke)
5c2b3cd4b856f1bb536daaf7f576b1b1b42293ca dbwrapper: Use DataStream for batch operations (TheCharlatan)
Pull request description:
This refactor is required for https://github.com/bitcoin/bitcoin/pull/28052 and https://github.com/bitcoin/bitcoin/pull/28451
Thus, split it out.
ACKs for top commit:
ajtowns:
utACK fa19c914f7fe7be127c0fb330b41ff7c091f40aa
TheCharlatan:
ACK fa19c914f7fe7be127c0fb330b41ff7c091f40aa
Tree-SHA512: d9c232324702512e45fd73ec3e3170f1e8a8c8f9c49cb613a6b693a9f83358914155527ace2517a2cd626a0cedcada56ef70a2a7812edafb1888fd6765eebba2
Follow-up from #27021.
Also included is an ASCII art visualization of the test’s transaction
topology by theStack.
Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
Follow-up from #27021: In the prior commit, the vector started counting
at 0, but the transaction names started with 1. This commit matches the
names to the transactions’ vector indices for better readability.
Co-authored-by: theStack <sebastian.falbesoner@gmail.com>
32c1dd1ad65af0ad4d36a56d2ca32a8481237e68 [test] mempool coins disappearing mid-package evaluation (glozow)
a67f460c3fd1c7eb8070623666d887eefccff0d6 [refactor] split setup in mempool_limit test (glozow)
d08696120e3647b4c2cd0ae8d6e57dea12418b7c [test framework] add ability to spend only confirmed utxos (glozow)
3ea71feb11c261f002ed918f91f3434fd8a23589 [validation] don't LimitMempoolSize in any subpackage submissions (glozow)
d227b7234cd4cfd7c593ffcf8e2f24573d1ebea5 [validation] return correct result when already-in-mempool tx gets evicted (glozow)
9698b81828ff98820fa49c83ca364063233374c6 [refactor] back-fill results in AcceptPackage (glozow)
8ad7ad33929ee846a55a43c55732be0cb8973060 [validation] make PackageMempoolAcceptResult members mutable (glozow)
03b87c11ca0705e1d6147b90da33ce555f9f41c8 [validation] add AcceptSubPackage to delegate Accept* calls and clean up m_view (glozow)
3f01a3dab1c4ee37fd4093b6a0a3b622f53e231d [CCoinsViewMemPool] track non-base coins and allow Reset (glozow)
7d7f7a1189432b1b6245ba25df572229870567cb [policy] check for duplicate txids in package (glozow)
Pull request description:
While we are evaluating a package, we split it into "subpackages" for evaluation (currently subpackages all have size 1 except the last one). If a subpackage has size 1, we may add a tx to mempool and call `LimitMempoolSize()`, which evicts transactions if the mempool gets full. We handle the case where the just-submitted transaction is evicted immediately, but we don't handle the case in which a transaction from a previous subpackage (either just submitted or already in mempool) is evicted. Mainly, since the coins created by the evicted transaction are cached in `m_view`, we don't realize the UTXO has disappeared until `CheckInputsFromMempoolAndCache` asserts that they exist. Also, the returned `PackageMempoolAcceptResult` reports that the transaction is in mempool even though it isn't anymore.
Fix this by not calling `LimitMempoolSize()` until the very end, and editing the results map with "mempool full" if things fall out.
Pointed out by instagibbs in faeed687e5 on top of the v3 PR.
ACKs for top commit:
instagibbs:
reACK 32c1dd1ad6
Tree-SHA512: 61e7f69db4712e5e5bfa27d037ab66bdd97f1bf60a8d9ffb96adb1f0609af012c810d681102ee5c7baec7b5fe8cb7c304a60c63ccc445d00d86a2b7f0e7ddb90
(1) Call AcceptSingleTransaction when there is only 1 transaction in the
subpackage. This avoids calling PackageMempoolChecks() which enforces
rules that don't need to be applied for a single transaction, i.e.
disabling CPFP carve out.
There is a slight change in the error type returned, as shown in the
txpackage_tests change. When a transaction is the last one left in the
package and its fee is too low, this returns a PCKG_TX instead of
PCKG_POLICY. This interface is clearer; "package-fee-too-low" for 1
transaction would be a bit misleading.
(2) Clean up m_view and m_viewmempool so that coins created in this
sub-package evaluation are not available for other sub-package
evaluations. The contents of the mempool may change, so coins that are
available now might not be later.
Duplicates of normal transactions would be found by looking for
conflicting inputs, but this doesn't catch identical empty transactions.
These wouldn't be valid but exiting early is good and AcceptPackage's
result sanity checks assume non-duplicate transactions.
The protocol.h file contains many non-consensus related definitions and
should thus not be part of the libbitcoinkernel. This commit makes
protocol.h no longer a required include for users of the
libbitcoinkernel.
This commit is part of the libbitcoinkernel project, namely its stage 1
step 3: Decouple most non-consensus headers from libbitcoinkernel.
Co-Authored-By: Cory Fields <cory-nospam-@coryfields.com>
While touching all constructors in the previous commit, the class name
can be adjusted to comply with the style guide.
-BEGIN VERIFY SCRIPT-
sed -i 's/CBufferedFile/BufferedFile/g' $( git grep -l CBufferedFile )
-END VERIFY SCRIPT-
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.
e73d2a8018def940afadb5d699b18f39e882c1fc refactor: remove clientversion include from dbwrapper.h (Cory Fields)
4240a082b81d8ceb7615b1b4ca0d2857382f317b refactor: Use DataStream now that version/type are unused (Cory Fields)
f15f790618d328abd207d55e6291229eb2a8643f 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 e73d2a8018def940afadb5d699b18f39e882c1fc
ajtowns:
reACK e73d2a8018def940afadb5d699b18f39e882c1fc
Tree-SHA512: 45b0dd7c2e918493e2ee92a8e35320ad17991cb8908cb811150a96c5fd584ce177c775baeeb8675a602c90b9ba9203b8cefc0a2a0c6a71078b1d9c2b41e1f3ba
db9888feec48c6220a2fcf92865503bbbdab02a4 net: detect wrong-network V1 talking to V2Transport (Pieter Wuille)
91e1ef8684997fb4b3e8b64ef3935a936445066b test: add unit tests for V2Transport (Pieter Wuille)
297c8889975a18258d6cc39b1ec1e94fed6630fb net: make V2Transport preallocate receive buffer space (Pieter Wuille)
3ffa5fb49ee4a6d9502aa957093bd94058630282 net: make V2Transport send uniformly random number garbage bytes (Pieter Wuille)
0be752d9f8ca27320bc3e82498c7640fabd7e8de net: add short message encoding/decoding support to V2Transport (Pieter Wuille)
8da8642062fa2c7aa2f49995b832c3d0897e37ed net: make V2Transport auto-detect incoming V1 and fall back to it (Pieter Wuille)
13a7f01557272db652b3f333af3f06af6897253f net: add V2Transport class with subset of BIP324 functionality (Pieter Wuille)
dc2d7eb810ef95b06620f334c198687579916435 crypto: Spanify EllSwiftPubKey constructor (Pieter Wuille)
5f4b2c6d79e81ee0445752ad558fcc17831f4b2f net: remove unused Transport::SetReceiveVersion (Pieter Wuille)
c3fad1f29df093e8fd03d70eb43f25ee9d531bf7 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 db9888feec48c6220a2fcf92865503bbbdab02a4
theStack:
re-ACK db9888feec48c6220a2fcf92865503bbbdab02a4
mzumsande:
Code Review ACK db9888feec48c6220a2fcf92865503bbbdab02a4
Tree-SHA512: 8906ac1e733a99e1f31c9111055611f706d80bbfc2edf6a07fa6e47b21bb65baacd1ff17993cbbf588063b2f5ad30b3af674a50c7bc8e8ebf4671483a21bbfeb
1580e3be83bd03985b2f288ed70de510903068d9 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 1580e3be83bd03985b2f288ed70de510903068d9
Tree-SHA512: c87c8bcd1f6b3a97ef772be93102efb912811c59f32211cfd531a116f1da8a57c8c6ff106b34f2a2b88d8b34fb5bc30d9f9ed6d2720113ffcaaa2f8d5dc9eb27
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".
fa626af3edbe8d98b2de91dd71729ceef90389fb Remove unused legacy CHashVerifier (MarcoFalke)
fafa3fc5a62702da72991497e3270034eb9159c0 test: add tests that exercise WithParams() (MarcoFalke)
fac81affb527132945773a5315bd27fec61ec52f Use serialization parameters for CAddress serialization (MarcoFalke)
faec591d64e40ba7ec7656cbfdda1a05953bde13 Support for serialization parameters (MarcoFalke)
fac42e9d35f6ba046999b2e3a757ab720c51b6bb Rename CSerAction* to Action* (MarcoFalke)
aaaa3fa9477eef9ea72e4a501d130c57b47b470a 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 fa626af3edbe8d98b2de91dd71729ceef90389fb
ajtowns:
ACK fa626af3edbe8d98b2de91dd71729ceef90389fb
Tree-SHA512: 229d379da27308890de212b1fd2b85dac13f3f768413cb56a4b0c2da708f28344d04356ffd75bfcbaa4cabf0b6cc363c4f812a8f1648cff9e436811498278318
583af18fd1d0bda5a6a1d0403ffc498a512a546d 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 583af18fd1d0bda5a6a1d0403ffc498a512a546d
brunoerg:
crACK 583af18fd1d0bda5a6a1d0403ffc498a512a546d
Tree-SHA512: 58a178432ba1eb0a2f7597b6700e96477e8b10f429ef642445a52db12e74d81aec307888315b772bfda9610f90df3e1d556cf024c2bef1d520303b12584feaaa
10546a569c6c96a5ec1b9708abf9ff5c8644f669 wallet: accurately account for the size of the witness stack (Antoine Poinsot)
9b7ec393b82ca9d7ada77d06e0835df0386a8b85 wallet: use descriptor satisfaction size to estimate inputs size (Antoine Poinsot)
8d870a98731e8db5ecc614bb5f7c064cbf30c7f4 script/signingprovider: introduce a MultiSigningProvider (Antoine Poinsot)
fa7c46b503f0b69630f55dc43021d2099e3515ba descriptor: introduce a method to get the satisfaction size (Antoine Poinsot)
bdba7667d2d65f31484760a8e8420c488fc5f801 miniscript: introduce a helper to get the maximum witness size (Antoine Poinsot)
4ab382c2cdb09fb4056711b4336807845cbe1ad5 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 10546a569c6c96a5ec1b9708abf9ff5c8644f669
achow101:
re-ACK 10546a569c6c96a5ec1b9708abf9ff5c8644f669
Tree-SHA512: 43ed1529fbd30af709d903c8c5063235e8c6a03b500bc8f144273d6184e23a53edf0fea9ef898ed57d8a40d73208b5d935cc73b94a24fad3ad3c63b3b2027174
fae405556d56f6f13ce57f69a06b9ec1e825422b scripted-diff: Rename CBlockTreeDB -> BlockTreeDB (MarcoFalke)
faf63039cce40f5cf8dea5a1d24945773c3433a1 Fixup style of moved code (MarcoFalke)
fa65111b99627289fd47dcfaa5197e0f09b8a50e move-only: Move CBlockTreeDB to node/blockstorage (MarcoFalke)
fa8685597e7302fc136f21b6dd3a4b187fa8e251 index: Drop legacy -txindex check (MarcoFalke)
fa69148a0a26c5054dbccdceeac8e117bf449275 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 fae405556d56f6f13ce57f69a06b9ec1e825422b, though I lack historical context to really judge the second commit fa8685597e7302fc136f21b6dd3a4b187fa8e251.
stickies-v:
ACK fae405556d56f6f13ce57f69a06b9ec1e825422b
Tree-SHA512: 9da8f48767ae52d8e8e21c09a40c949cc0838794f1856cc5f58a91acd3f00a3bca818c8082242b3fdc9ca5badb09059570bb3870850d3807b75a8e23b5222da1
This also cleans up the addrman (de)serialization code paths to only
allow `Disk` serialization. Some unit tests previously forced a
`Network` serialization, which does not make sense, because Bitcoin Core
in production will always `Disk` serialize.
This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
Towns.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
Today, this code only has one spot where it needs well-formed pubkeys,
but future PRs will want to reuse this code.
Add a function which creates a well-formed byte array that can be turned
into a pubkey. It is not required that the pubkey is valid, just that it
can be recognized as a compressed or uncompressed pubkey.
Note: while the main intent of this commit is to wrap the existing
logic into a function, it also switches to `PickValueFromArray` so that
we are only choosing one of 0x04, 0x06, or 0x07. The previous code,
`ConsumeIntegralInRange` would have also picked 0x05, which is not
definied in the context of compressed vs uncompressed keys.
See https://bitcoin.stackexchange.com/questions/57855/c-secp256k1-what-do-prefixes-0x06-and-0x07-in-an-uncompressed-public-key-signif
for more details.
When estimating the maximum size of an input, we were assuming the
number of elements on the witness stack could be encode in a single
byte. This is a valid approximation for all the descriptors we support
(including P2WSH Miniscript ones), but may not hold anymore once we
support Miniscript within Taproot descriptors (since the max standard
witness stack size of 100 gets lifted).
It's a low-hanging fruit to account for it correctly, so just do it now.
Instead of using the dummysigner to compute a placeholder satisfaction,
infer a descriptor on the scriptPubKey of the coin being spent and use
the estimation of the satisfaction size given by the descriptor
directly.
Note this (almost, see next paragraph) exactly conserves the previous
behaviour. For instance CalculateMaximumSignedInputSize was previously
assuming the input to be spent in a transaction that spends at least one
Segwit coin, since it was always accounting for the serialization of the
number of witness elements.
In this commit we use a placeholder for the size of the serialization of
the witness stack size (1 byte). Since the logic in this commit is
already tricky enough to review, and that it is only a very tiny
approximation not observable through the existing tests, it is addressed
in the next commit.
In the wallet code, we are currently estimating the size of a signed
input by doing a dry run of the signing logic. This is unnecessary as
all outputs we are able to sign for can be represented by a descriptor,
and we can derive the size of a satisfaction ("signature") from the
descriptor itself directly.
In addition, this approach does not scale: getting the size of a
satisfaction through a dry run of the signing logic is only possible for
the most basic scripts.
This commit introduces the computation of the size of satisfaction per
descriptor. It's a bit intricate for 2 main reasons:
- We want to conserve the behaviour of the current dry-run logic used by
the wallet that sometimes assumes ECDSA signatures will be low-r,
sometimes not (when we don't create them).
- We need to account for the witness discount. A single descriptor may
sometimes benefit of it, sometimes not (for instance `pk()` if used as
top-level versus if used inside `wsh()`).
