- 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
Without ffile-prefix-map, the debug symbols will contain paths for the
guix store which will include the hashes of each package. However, the
hash for the same package will differ when on different architectures.
In order to be reproducible regardless of the architecture used to build
the package, map all guix store prefixes to something fixed, e.g. /usr.
We might be able to drop this in favour of using --with-nonshared-cflags
when we being using newer versions of glibc.
Without ffile-prefix-map, the debug symbols will contain paths for the
guix store which will include the hashes of each package. However, the
hash for the same package will differ when on different architectures.
In order to be reproducible regardless of the architecture used to build
the package, map all guix store prefixes to something fixed, e.g. /usr.
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
e410144fc4 doc: Fix typo in macdeploy (Jeremy Rand)
Pull request description:
The text of the link used an underscore, while the URL used a space. The latter is correct; the former yields zero results on Apple's website.
ACKs for top commit:
shaavan:
ACK e410144fc4
Tree-SHA512: b242aa7a9e93cf2609f13247d6efe2a5ec9c8b20c95b11a4c22e98be1afd4beba49041d397432ddbfeeeb6b95a2be23db8a3bbe3bce088eb402c7947e64f6ffd
f665c6ecda test: fix failing test interface_usdt_utxocache.py (Sebastian Falbesoner)
Pull request description:
The `from_node` argument doesn't exist anymore for `MiniWallet.create_self_transfer` since PR #25435 (commit fa8421bc5b), leading to an error on master:
```
$ sudo ./test/functional/interface_usdt_utxocache.py
2022-06-27T17:45:35.585000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_7s1djjo1
2022-06-27T17:45:36.515000Z TestFramework (INFO): testing the utxocache:uncache tracepoint API
2022-06-27T17:45:36.517000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/home/honeybadger/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
self.run_test()
File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 149, in run_test
self.test_uncache()
File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 172, in test_uncache
invalid_tx = self.wallet.create_self_transfer(
TypeError: create_self_transfer() got an unexpected keyword argument 'from_node'
2022-06-27T17:45:36.568000Z TestFramework (INFO): Stopping nodes
[...]
```
Fix this by removing the argument. (Unfortunately, the USDT tests don't seem to run on any CI target, I guess that's due to missing permissions to hook into the kernel.)
ACKs for top commit:
MarcoFalke:
cr ACK f665c6ecda
Tree-SHA512: 74f8e398739a25ab5518ff71b998d03d4e529a786ba5b424509de81a511ad3e2e1cd38a5b7bb9f1f5a21340391d6807f4951ff39fa3a2ad65a3b11b989eebea6
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".
b1d2fb4ceb Revert "ci: Increase CPU number for "Win64 native" task" (Hennadii Stepanov)
Pull request description:
This reverts commit 849cf967a3.
It seems this should [improve](https://github.com/bitcoin/bitcoin/pull/25460#discussion_r907268911) the total CI throughput.
ACKs for top commit:
MarcoFalke:
review ACK b1d2fb4ceb
Tree-SHA512: 833665bbea00b695dd1c600c6a47943d47b1a2a3560886b2056544cf62542d6f91b1f2e157ad082b02b67922f1fff14029da35b1a9d85339c2bf526f41b41836
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
fa83c0c44f test: Remove unused call to generate in rpc_mempool_info (MacroFake)
fa13375aa3 test: Sync MiniWallet utxo state after each generate call (MacroFake)
dddd7c4d39 test: Drop spent utxos in MiniWallet scan_tx (MacroFake)
fa04ff61b6 test: Return new_utxos from create_self_transfer_multi in MiniWallet (MacroFake)
fa34e44e98 test: Return new_utxo from create_self_transfer in MiniWallet (MacroFake)
Pull request description:
I need this for some stuff, but it also makes sense on its own to:
* unify the flow with a private `_create_utxo` helper
* simplify the flow by giving the caller ownership of the utxo right away
ACKs for top commit:
kouloumos:
re-ACK fa83c0c44f🚀
Tree-SHA512: 381f0e814864ba207363a56859a6c0519e4a86d0562927f16a610a5b706c9fc942c1b5e93388cda0fa0b3cacd58f55bc2ffcc60355858a580263e5bef33c2f4b
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
880d4aaf81 build: use BOOST_NO_CXX98_FUNCTION_BASE to suppress warnings (fanquake)
1bdbbbdc46 build: suppress array-bounds errors in libxkbcommon (fanquake)
Pull request description:
2 changes to better support building with GCC 12, which out of the box, is currently broken if you want to build using depends.
Prevent `-Warray-bounds` errors when building libxkbcommon. i.e:
```bash
src/xkbcomp/ast-build.c:82:27: error: array subscript 'ExprDef[0]' is partly outside array bounds of 'unsigned char[32]' [-Werror=array-bounds]
82 | expr->expr.value_type = type;
| ~~~~~~~~~~~~~~~~~~~~~~^~~~~~
src/xkbcomp/ast-build.c:75:21: note: object of size 32 allocated by 'malloc'
75 | ExprDef *expr = malloc(size);
| ^~~~~~~~~~~~
```
It might be the case that these would be fixed by updating the
package, but that would also require installing new build tools (meson),
as well as potentially more dependencies (wayland), and it'd need
testing with Qt. For now, just turn the errors into wanrings.
Define `BOOST_NO_CXX98_FUNCTION_BASE` to prevent GCC warning about the use of `std::unary_function`. i.e:
```bash
/bitcoin/depends/aarch64-unknown-linux-gnu/include/boost/container_hash/hash.hpp:131:33:
warning: 'template<class _Arg, class _Result> struct std::unary_function' is deprecated [-Wdeprecated-declarations]
131 | struct hash_base : std::unary_function<T, std::size_t> {};
| ^~~~~~~~~~~~~~
In file included from /usr/include/c++/12/bits/unique_ptr.h:37,
from /usr/include/c++/12/memory:76,
from ./init.h:10,
from init.cpp:10:
/usr/include/c++/12/bits/stl_function.h:117:12: note: declared here
117 | struct unary_function
```
Boost `container_hash` (included via functional -> multi_index) uses
[`std::unary_function`, which was deprecated in C++11](https://en.cppreference.com/w/cpp/utility/functional/unary_function), and "removed" in
C++17. It's use causes warnings with newer compilers, i.e GCC 12.1.
Use the MACRO outlined in https://github.com/boostorg/container_hash/issues/22, and added to Boost Config for GCC 12 in https://github.com/boostorg/config/pull/430, to prevent it's use.
[BOOST_NO_CXX98_FUNCTION_BASE](https://www.boost.org/doc/libs/master/libs/config/doc/html/boost_config/boost_macro_reference.html):
> The standard library no longer supports std::unary_function and std::binary_function.
> They were deprecated in C++11 and is removed from C++14.
Guix Build (x86_64):
```bash
```
Guix Build (arm64):
```bash
```
ACKs for top commit:
laanwj:
Code review ACK 880d4aaf81
Tree-SHA512: 10c4679c3eb788e9279acc4960731c55ae1568bd3df525d3c46f97d8b0319e7d8450b1638b6777d98111b5991dba5c787e95d80b1ac932e0b4779d4b8e74875e
