Since:
- UpdateTransactionsFromBlock is only called by
MaybeUpdateMempoolForReorg, which calls it with the gArgs-determined
ancestor limits
- UpdateForDescendants is only called by UpdateTransactionsFromBlock
with the ancestor limits unchanged
We can remove the requirement to specify the ancestor limits for both
UpdateTransactionsFromBlock and UpdateForDescendants and just use the
values in the m_limits member.
Also move some removed comments to MemPoolLimits struct members.
The uint64_t cast in UpdateForDescendants is not new behavior,
see the diff in CChainState::MaybeUpdateMempoolForReorg for where they
were previously.
Better to be explicit when it comes to sizes to avoid unintentional
bugs. We use MB and KB all over the place.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_(ANCESTOR|DESCENDANT)_SIZE_LIMIT" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_KVB@g"
-END VERIFY SCRIPT-
Better to be explicit when it comes to time to avoid unintentional bugs.
-BEGIN VERIFY SCRIPT-
find_regex="DEFAULT_MEMPOOL_EXPIRY" \
&& git grep -l -E "$find_regex" \
| xargs sed -i -E "s@$find_regex@\0_HOURS@g"
-END VERIFY SCRIPT-
- Store the mempool expiry (-mempoolexpiry) in CTxMemPool as a
std::chrono::seconds member.
- Remove the requirement to explicitly specify a mempool expiry for
LimitMempoolSize(...), just use the newly-introduced member.
- Remove all now-unnecessary instances of:
std::chrono::hours{gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY)}
The version of GetMinFee() with a custom size specification is and
should only be used by tests. Mark it as protected and use a derived
class exposing GetMinFee() as public in tests.
- Store the mempool size limit (-maxmempool) in CTxMemPool as a member.
- Remove the requirement to explicitly specify a mempool size limit for
CTxMemPool::GetMinFee(...) and LimitMempoolSize(...), just use the
stored mempool size limit where possible.
- Remove all now-unnecessary instances of:
gArgs.GetIntArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE_MB) * 1000000
The code change in CChainState::GetCoinsCacheSizeState() is correct
since the coinscache should not repurpose "extra" mempool memory
headroom for itself if the mempool doesn't even exist.
Reviewers: Note that CTxMemPool now requires a non-defaulted
CTxMemPool::Options for its constructor. Meaning that there's no need to
worry about a stray CTxMemPool constructor somewhere defaulting to
something incorrect. All instances of CTxMemPool construction are
addressed here in this commit.
We set options for CTxMemPool and construct it in many different ways. A
good example can be seen in how we determine CTxMemPool's check_ratio in
AppInitMain(...).
1. We first set the default based on chainparams's
DefaultConsistencyChecks()
2. Then, we apply the ArgsManager option on top of that default
3. Finally, we clamp the result of that between 0 and 1 Million
With this patch, most CTxMemPool construction are along the lines of:
MemPoolOptions mempool_opts{...default overrides...};
ApplyArgsManOptions(argsman, mempool_opts);
...hard overrides...
CTxMemPool pool{mempool_opts};
This "compositional" style of building options means that we can omit
unnecessary/irrelevant steps wherever we want but also maintain full
customizability.
For example:
- For users of libbitcoinkernel, where we eventually want to remove
ArgsManager, they simply won't call (or even know about)
ApplyArgsManOptions.
- See src/init.cpp to see how the check_ratio CTxMemPool option works
after this change.
A MemPoolOptionsForTest helper was also added and used by tests/fuzz
tests where a local CTxMemPool needed to be created.
The change in src/test/fuzz/tx_pool.cpp seemingly changes behaviour by
applying ArgsManager options on top of the CTxMemPool::Options defaults.
However, in future commits where we introduce flags like -maxmempool,
the call to ApplyArgsManOptions is actually what preserves the existing
behaviour. Previously, although it wasn't obvious, our CTxMemPool would
consult gArgs for flags like -maxmempool when it needed it, so it
already relied on ArgsManager information. This patchset just laid bare
the obfuscatory perils of globals.
[META] As this patchset progresses, we will move more and more
CTxMemPool-relevant options into MemPoolOptions and add their
ArgsMan-related logic to ApplyArgsManOptions.
e673d8b475 bench: Enable loading benchmarks depending on what's compiled (Andrew Chow)
4af3547eba bench: Use mock wallet database for wallet loading benchmark (Andrew Chow)
49910f255f sqlite: Use in-memory db instead of temp for mockdb (Andrew Chow)
a1080802f8 walletdb: Create a mock database of specific type (Andrew Chow)
7c0d34476d bench: reduce the number of txs in wallet for wallet loading bench (Andrew Chow)
f85b54ed27 bench: Add transactions directly instead of mining blocks (Andrew Chow)
d94244c4bf bench: reduce number of epochs for wallet loading benchmark (Andrew Chow)
817c051364 bench: use unsafesqlitesync in wallet loading benchmark (Andrew Chow)
9e404a9831 bench: Remove minEpochIterations from wallet loading benchmark (Andrew Chow)
Pull request description:
`minEpochIterations` is probably unnecessary to set, so removing it makes the runtime much faster.
ACKs for top commit:
Rspigler:
tACK e673d8b475
furszy:
Code review ACK e673d8b4, nice PR.
glozow:
Concept ACK e673d8b475. For each commit, verified that there was a performance improvement without negating the purpose of the bench, and made some effort to verify that the code is correct.
Tree-SHA512: 9337352ef846cf18642d5c14546c5abc1674b4975adb5dc961a1a276ca91f046b83b7a5e27ea6cd26264b96ae71151e14055579baf36afae7692ef4029800877
fa956e7508 Replace CountSecondsDouble with Ticks<SecondsDouble> (MacroFake)
Pull request description:
Seems odd to have two ways to say exactly the same thing when one is sufficient.
ACKs for top commit:
fanquake:
ACK fa956e7508
shaavan:
ACK fa956e7508
w0xlt:
ACK fa956e7508
Tree-SHA512: b599470e19b693da1ed1102d1e86b08cb03adaddf2048752b6d050fdf86055be117ff0ae10b6953d03e00eaaf7b0cfa350137968b67d6c5b3ca68c5aa50ca6aa
fa1fe2e500 Remove LOCKTIME_MEDIAN_TIME_PAST constant (MarcoFalke)
Pull request description:
The constant is exposed in policy code, which doesn't make sense:
* Wallet and mempool need to assume the flag to be always active to function properly.
* Setting (or unsetting) the flag has no effect on policy code.
The constant is only used in `ContextualCheckBlock` (consensus code) to set a flag and then read the flag again. I think this can be better achieved by using a `bool`. If there is a need to use a flag in the future, it will be trivial to do so then.
(The previous use for the constant was removed in df562d698a)
ACKs for top commit:
Sjors:
utACK fa1fe2e500
glozow:
code review ACK fa1fe2e500, AFAICT this is safe and makes sense as `SequenceLocks` doesn't use it, wallet/ATMP no longer need it since #24080, and `ContextualCheckBlock` effectively uses it as a roundabout boolean.
instagibbs:
utACK fa1fe2e500
Tree-SHA512: de1972498c545d608a09630d77d8c7e38ed50a6ec40d6c0d720310a1647ed5b48b4ace0078c80db10e7f97aacc552fffae251fe3256e9a19a908b933ba2dc552
b80de4c505 test: Test signing psbts without explicitly having scripts (Andrew Chow)
a73b56888a wallet: also search taproot pubkeys in FillPSBT (Andrew Chow)
6cff82722f sign: Use sigdata taproot spenddata when signing (Andrew Chow)
5f12fe3f36 psbt: Implement merge for Taproot fields (Andrew Chow)
1ece9a3715 psbt, test: Check for taproot fields in taproot psbt test (Andrew Chow)
496a1bbe5e taproot: Use pre-existing signatures if available (Andrew Chow)
0ad21e7c55 tests: Test taproot fields for PSBT (Andrew Chow)
103c6fd279 psbt: Remove non_witness_utxo for segwit v1+ (Andrew Chow)
7dccdd3157 Implement decodepsbt for Taproot fields (Andrew Chow)
ac7747585f Fill PSBT Taproot output data to/from SignatureData (Andrew Chow)
25b6ae46e7 Assert that TaprootBuilder is Finalized during GetSpendData (Andrew Chow)
3ae5b6af21 Store TaprootBuilder in SigningProviders instead of TaprootSpendData (Andrew Chow)
4d1223e512 Fetch key origins for Taproot keys (Andrew Chow)
52e3f2f88e Fill PSBT Taproot input data to/from SignatureData (Andrew Chow)
05e2cc9a30 Implement de/ser of PSBT's Taproot fields (Andrew Chow)
d557eff2ad Add serialization methods to XOnlyPubKey (Andrew Chow)
d43923c381 Add TaprootBuilder::GetTreeTuples (Andrew Chow)
ce911204e4 Move individual KeyOriginInfo de/ser to separate function (Andrew Chow)
Pull request description:
Implements the Taproot fields for PSBT described in [BIP 371](https://github.com/bitcoin/bips/blob/master/bip-0371.mediawiki).
ACKs for top commit:
laanwj:
Code review ACK b80de4c505
Tree-SHA512: 50b79bb44f353c9ec2ef4c98aac08a81eba560987e5264a5684caa370e9c4e7a8255c06747fc47749511be45b32d01492e015f92b82be8d22bc8bf192073bd26
b2733ab6a8 net: add new method Sock::Listen() that wraps listen() (Vasil Dimov)
3ad7de225e net: add new method Sock::Bind() that wraps bind() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Add new methods `Sock::Bind()` and `Sock::Listen()` that wrap `bind()` and `listen()`.
This will help to increase `Sock` usage and make more code mockable.
ACKs for top commit:
pk-b2:
ACK b2733ab6a8
laanwj:
Code review ACK b2733ab6a8
Tree-SHA512: c6e737606703e2106fe60cc000cfbbae3a7f43deadb25f70531e2cac0457e0b0581440279d14c76c492eb85c12af4adde52c30baf74542c41597e419817488e8
a8d6abba5e net: change GetBindAddress() to take Sock argument (Vasil Dimov)
748dbcd9f2 net: add new method Sock::GetSockName() that wraps getsockname() (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
Wrap the syscall `getsockname()` in `Sock::GetSockName()` and change `GetBindAddress()` to take a `Sock` argument so that it can use the wrapper.
This further encapsulates syscalls inside the `Sock` class and makes the callers mockable.
ACKs for top commit:
laanwj:
Code review ACK a8d6abba5e
Tree-SHA512: 3a73463258c0057487fb3fd67215816b03a1c5160f45e45930eaeef86bb3611ec385794cdb08339aa074feba8ad67cd2bfd3836f6cbd40834e15d933214a05dc
baf4efe02f rpc: use enum instead of string for filter type (w0xlt)
Pull request description:
This PR changes the `getblockfilter` RPC to use `BlockFilterType` enum instead of a repeated string for `filtertype_name`.
ACKs for top commit:
furszy:
ACK baf4efe0
brunoerg:
ACK baf4efe02f
Tree-SHA512: 31c79c0a5f0b17fd69b399bb026f523003b656733d6b7d5ffe665921a8cc0f1e0334d2e465145cd89fbd85e196059cf56f4f11563bbc92948b0606080ca76524
When filling a PSBT, we search the listed pubkeys in order to determine
whether the current DescriptorScriptPubKeyMan could sign the transaction
even if it is not watching the scripts. With Taproot, the taproot
pubkeys need to be searched as well.
The taproot spenddata stored in a sigdata is the combination of data
existing previously (e.g. in a PSBT) and the data stored in a
SigningProvider. In order to use the external data when signing, we need
to be using the sigdata's spenddata.
GetSpendData needs to be finalized in order to be used. To avoid future
bugs, assert `!m_output_key.IsNull()` as m_output_key is only set during
Finalize.
TaprootSpendData can be gotten from TaprootBuilder, however for PSBT, we
also need TaprootBuilders directly (for the outputs). So we store the
TaprootBuilder in the FlatSigningProvider and when the TaprootSpendData
is needed, we generate it on the fly using the stored builder.
It is useful to have serialzation methods for XOnlyPubKey. These will
serialize the internal uint256, so it is not prefixed with the length as
CPubKey does.
GetTreeTuples returns the leaves in DFS order as tuples of depth, leaf
version, and script. This is a representation of the tree that can be
serialized.
To make it easier to de/serialize individual KeyOriginInfo for PSBTs,
separate the actual de/serialization of KeyOriginInfo to its own
function.
This is an additional separation where any length prefix is processed by
the caller.
MarcoFalke mentioned that this is likely a bug since "any log messages
should be muted, not accumulated and turned into an OOM when fuzzing for
a long time".
e357c89538 p2p, doc: Use MAX_BLOCKS_TO_ANNOUNCE consistently (Martin Zumsande)
Pull request description:
Block announcements via headers may have up to `MAX_BLOCKS_TO_ANNOUNCE = 8` entries according to the definition of this constant.
However, there are a few spots saying they should have a size _less than_ `MAX_BLOCKS_TO_ANNOUNCE`. Fix these.
I don't think that this is critical (this only changes behavior when we get a headers announcement with exactly `MAX_BLOCKS_TO_ANNOUNCE` blocks which we can't connect), but it would be nice to handle this limit consistently.
ACKs for top commit:
dergoegge:
utACK e357c89538 - This PR makes the usage and docs of `MAX_BLOCKS_TO_ANNOUNCE` consistent with its description.
Tree-SHA512: f3772026ab0f402e3a551127ef6e4a98fa9e7af250715fe317c05988b5b33f2f3e098a00e03960d4d28c8bd2b7a97231f7f99f22f1c152c000b2e27b658cf8f2
fa8aa0aa81 Pass Peer& to Misbehaving() (MacroFake)
Pull request description:
`Misbehaving` has several coding related issues (ignoring the conceptual issues here for now):
* It is public, but it is not supposed to be called from outside of net_processing. Fix that by making it private and creating a public `UnitTestMisbehaving` method for unit testing only.
* It doesn't do anything if a `nullptr` is passed. It would be less confusing to just skip the call instead. Fix that by passing `Peer&` to `Misbehaving()`.
* It calls `GetPeerRef`, causing `!m_peer_mutex` lock annotations to be propagated. This is harmless, but verbose. Fix it by removing the no longer needed call to `GetPeerRef` and the no longer needed lock annotations.
ACKs for top commit:
vasild:
ACK fa8aa0aa81
w0xlt:
Code Review ACK fa8aa0aa81
Tree-SHA512: e60a6b317f2b826f9e0724285d00b632d3e2a91ded9fa5ba01c80766c5d39270b719be234c01302d46eaba600910032693836aa116ff05ee1b590c7530881cd3
fa07f84e31 Fix signed integer overflow in prioritisetransaction RPC (MarcoFalke)
fa52cf8e11 refactor: Replace feeDelta by m_modified_fee (MarcoFalke)
Pull request description:
Signed integer overflow is UB in theory, but not in practice. Still,
it would be nice to avoid this UB to allow Bitcoin Core to be
compiled with sanitizers such as `-ftrapv` or ubsan.
It is impossible to predict when and if an overflow occurs, since
the overflow caused by a prioritisetransaction RPC might only be
later hit when descendant txs are added to the mempool.
Since it is impossible to predict reliably, leave it up to the user
to use the RPC endpoint responsibly, considering their mempool
limits and usage patterns.
Fixes: #20626Fixes: #20383Fixes: #19278
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34146 / https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=47132
## Steps to reproduce
Build the code without the changes in this pull.
Make sure to pass the sanitizer flag:
```
./autogen.sh && ./configure --with-sanitizers=signed-integer-overflow && make clean && make -j $(nproc)
```
### Reproduce on RPC
```
./src/bitcoind -chain=regtest -noprinttoconsole &
./src/bitcoin-cli -chain=regtest prioritisetransaction 00000000deadbeef00000000deadbeef00000000deadbeef00000000deadbeef 0 9123456789123456789
./src/bitcoin-cli -chain=regtest prioritisetransaction 00000000deadbeef00000000deadbeef00000000deadbeef00000000deadbeef 0 9123456789123456789
|> txmempool.cpp:920:15: runtime error: signed integer overflow: 9123456789123456789 + 9123456789123456789 cannot be represented in type 'long int'
./src/bitcoin-cli -chain=regtest stop
```
### By fuzzing
```
wget https://github.com/bitcoin/bitcoin/files/8921302/clusterfuzz-testcase-minimized-validation_load_mempool-5599531390074880.bin.txt
FUZZ=validation_load_mempool ./src/test/fuzz/fuzz ./clusterfuzz-testcase-minimized-validation_load_mempool-5599531390074880.bin.txt
|> txmempool.cpp:920:15: runtime error: signed integer overflow: 7214801925397553184 + 2314885530818453536 cannot be represented in type 'long int'
|> validation_load_mempool: succeeded against 1 files in 0s.
ACKs for top commit:
vasild:
ACK fa07f84e31
dunxen:
ACK fa07f84
LarryRuane:
ACK fa07f84e31
Tree-SHA512: 4a357950af55a49c9113da0a50c2e743c5b752f0514dd8d16cd92bfde2f77dd0ef56aa98452626df6f7f7a5b51d1227021f6bc94091201a179f0d488ee32a0df
fafee78188 rpc: Return incrementalrelayfee in getmempoolinfo (MacroFake)
Pull request description:
Seems odd to return other policy info, but not the incremental relay fee
ACKs for top commit:
1440000bytes:
ACK fafee78188
w0xlt:
Code Review ACK fafee78188
jarolrod:
tACK fafee78188
Tree-SHA512: faad0af6c039b8257acbeac913bc5dcdb2ea2db304c95e52601536c8de60eb1186e9fbb4a64a68adf476605f18022aeda16a5644a0d7912592b0977e4c029638
fabae3541a rpc: Use steady_clock for getrpcinfo durations (MacroFake)
Pull request description:
Currently it uses `GetTimeMicros`, which is the system time. Using steady time instead, makes the code type safe and avoids spurious offsets when the system time adjusts.
ACKs for top commit:
laanwj:
Code review ACK fabae3541a
w0xlt:
Code Review ACK fabae3541a
shaavan:
Code Review ACK fabae3541a
Tree-SHA512: eb25fe3e69bf42ec8a2d4aaa69b435de7654b0d07218ce3e0c03ebaef6eb7f713128779057d012621773a34675a81f5757e7b2502c13b82adaf6e2df970d8c66
e4b4db5610 refactor: remove unused method `CDBWrapper::CompactRange` (Sebastian Falbesoner)
fb38c6e21f refactor: remove unused methods `{CDBIterator,CCoinsViewDBCursor}::GetValueSize()` (Sebastian Falbesoner)
Pull request description:
The `GetValueSize` methods haven't been used since the chainstate db cache has been switched from per-tx to per-txout model years ago (PR #10195, commit d342424301). The `CompactRange` is unused since the txindex migration code was removed (PR https://github.com/bitcoin/bitcoin/pull/22626, commit fa20f815a9).
ACKs for top commit:
fanquake:
ACK e4b4db5610
furszy:
re-ACK e4b4db56
laanwj:
Code review ACK e4b4db5610
Tree-SHA512: 77da445fb70c744046263c6f2ddb05782b68e3d4b2ea604dd7c7dc73ce7c1f2d2b48ec68db4dcb03e35fc27488b99b0a420f6fa3d5b83d325c1708ed68e99e0a
