fa4baf0756 fuzz: Rework ConsumeScript (MarcoFalke)
Pull request description:
This should make it easier for the fuzz engine to explore multisig code
paths. See discussion in https://github.com/bitcoin/bitcoin/issues/23105
The downside is that all fuzz inputs that use ConsumeScript are now
invalidated and need to be re-generated.
Another downside may be that most multisig scripts from ConsumeScript are
using likely not fully valid pubkeys.
ACKs for top commit:
jamesob:
ACK fa4baf0756
Tree-SHA512: 15814afdee76b05ff7a71c0f07bbd1b3cff30d709d5c1e68fd230c5f5d16e673e42709a4fab84d4a896bc27f972f917fe7c1d1b32c2bf4209658b18da97e478b
a032fa30d2 multiprocess: add interfaces::ExternalSigner class (Russell Yanofsky)
Pull request description:
Add `interfaces::ExternalSigner` class to let signer objects be passed between processes and let signer code run in the original process where the object was created.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
laanwj:
Concept and code review ACK a032fa30d2
hebasto:
re-ACK a032fa30d2
Tree-SHA512: 99a729fb3a64d010e142cc778a9f1f358e58345b77faaf2664de7d2277715d59df3352326e8f0f2a6628038670eaa4556310a549079fb28af6d2eeb05aea1460
214d9055ac fuzz: replace every fuzzer-controlled loop with a LIMITED_WHILE loop (Andrew Poelstra)
Pull request description:
Limits the number of iterations to 1000 rather than letting the fuzzer do millions or billions of iterations on a single core.
ACKs for top commit:
MarcoFalke:
cr ACK 214d9055ac
Tree-SHA512: 9741c32ccd126ea656e5c93371b7136eaa2f92dc9a490dd4d39642503b1a41174f3368245153e508c3b608fe37ab89800b67ada97b740e3b5a3728bb506429d3
4fe7cf1677 build: Drop unneeded dependencies for bitcoin-wallet tool (Hennadii Stepanov)
Pull request description:
`bitcoin-wallet` is an offline tool, and its code does not depend on networking stuff (ZMQ, UPnP, NAT-PMP, and LIBBITCOIN_SERVER).
Also `bitcoin-wallet` does not interacts with the chainstate, therefore dependency on LevelDB is not needed.
ACKs for top commit:
laanwj:
Code review ACK 4fe7cf1677
Tree-SHA512: 97342d9cdf8670806efe16dc7885a85ec92f3c1ae0819a4c3cc147938fc8642089e303c4432cb1395fc75b852c1af6a6a13fc58e29e027c23f75219fd3bd8cb4
Current CWalletTx state representation makes it possible to set
inconsistent states that won't be handled correctly by wallet sync code
or serialized & deserialized back into the same form.
For example, it is possible to call setConflicted without setting a
conflicting block hash, or setConfirmed with no transaction index. And
it's possible update individual m_confirm and fInMempool data fields
without setting an overall consistent state that can be serialized and
handled correctly.
Fix this without changing behavior by using std::variant, instead of an
enum and collection of fields, to represent sync state, so state
tracking code is safer and more legible.
This is a first step to fixing state tracking bugs
https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Wallet-Transaction-Conflict-Tracking,
by adding an extra margin of safety that can prevent new bugs from being
introduced as existing bugs are fixed.
faeb748f5b Use c++17 in clang-format (MarcoFalke)
Pull request description:
We currently use `Cpp11`, which "is a deprecated alias for `Latest`" according to https://clang.llvm.org/docs/ClangFormatStyleOptions.html . I doubt this has any effect, but I think for clarity setting to `c++17` make sense.
Also, remove unneeded settings:
* `ObjC*`, as we don't write objc code
* `Penalty`, as there is currently no line limit, so this has no effect
* `TabWidth`, as we don't use tabs
ACKs for top commit:
fanquake:
ACK faeb748f5b - we do have some Objc code, but it never changes.
Tree-SHA512: 50215849b3992e5841b45b281e68d4c58184a365cbaeec0d7adad844ad21672ae1b6247262047e2d7427835ef98fa9d9f83335696448c77303dde9ab0a121639
This fixes a null pointer crash in the bitcoin-gui PSBT dialog. The
bitcoin-gui interfaces::Node object has a null NodeContext pointer, and
can't broadcast transactions directly. It needs to broadcast
transactions through the bitcoin-node process instead.
Blindly chose a cap of 10000 iterations for every loop, except for
the two in script_ops.cpp and scriptnum_ops.cpp which appeared to
(sometimes) be deserializing individual bytes; capped those to one
million to ensure that sometimes we try working with massive scripts.
There was also one fuzzer-controlled loop in timedata.cpp which was
already capped, so I left that alone.
git grep 'while (fuzz' should now run clean except for timedata.cpp
libsecp256k1's secp256k1_schnorrsig_sign only follows BIP340 exactly
if an aux_rand32 argument is passed. When no randomness is used
(as is the case in the current codebase here), there is no impact
on security between not providing aux_rand32 at all, or providing
an empty one. Yet, for repeatability/testability it is simpler
to always use an all-zero one.
29173d6c6c ubsan: add minisketch exceptions (Cory Fields)
54b5e1aeab Add thin Minisketch wrapper to pick best implementation (Pieter Wuille)
ee9dc71c1b Add basic minisketch tests (Pieter Wuille)
0659f12b13 Add minisketch dependency (Gleb Naumenko)
0eb7928ab8 Add MSVC build configuration for libminisketch (Pieter Wuille)
8bc166d5b1 build: add minisketch build file and include it (Cory Fields)
b2904ceb85 build: add configure checks for minisketch (Cory Fields)
b6487dc4ef Squashed 'src/minisketch/' content from commit 89629eb2c7 (fanquake)
Pull request description:
This takes over #21859, which has [recently switched](https://github.com/bitcoin/bitcoin/pull/21859#issuecomment-921899200) to my integration branch. A few more build issues came up (and have been fixed) since, and after discussing with sipa it was decided I would open a PR to shepherd any final changes through.
> This adds a `src/minisketch` subtree, taken from the master branch of https://github.com/sipa/minisketch, to prepare for Erlay implementation (see #21515). It gets configured for just supporting 32-bit fields (the only ones we're interested in in the context of Erlay), and some code on top is added:
> * A very basic unit test (just to make sure compilation & running works; actual correctness checking is done through minisketch's own tests).
> * A wrapper in `minisketchwrapper.{cpp,h}` that runs a benchmark to determine which field implementation to use.
Only changes since my last update to the branch in the previous PR have been rebasing on master and fixing an issue with a header in an introduced file.
ACKs for top commit:
naumenkogs:
ACK 29173d6c6c
Tree-SHA512: 1217d3228db1dd0de12c2919314e1c3626c18a416cf6291fec99d37e34fb6eec8e28d9e9fb935f8590273b8836cbadac313a15f05b4fd9f9d3024c8ce2c80d02
2ec38bdebb Remove `gArgs` from `wallet.h` and `wallet.cpp` (Kiminuo)
Pull request description:
This is a follow-up PR to #22183 and is related to #21005 issue.
ACKs for top commit:
ryanofsky:
Code review ACK 2ec38bdebb. No changes since last review, just rebase
Tree-SHA512: ae7fa1927b0a268f25697928ccaf1b3cf10ee1ccef3f9d2344001fbd7e11fe8ce768745c65e76bd7d1632c6c7650612b5b54eaf2be61093854f75a4c4dcb1784
d150fe3ad5 refactor: use `CWallet` const shared pointers in dump{privkey,wallet} RPCs (Sebastian Falbesoner)
ec2792d1dc refactor: use const `LegacyScriptPubKeyMan` references in dump{privkey,wallet} RPCs (Sebastian Falbesoner)
29905c092f refactor: avoid multiple key->metadata lookups in dumpwallet RPC (Sebastian Falbesoner)
Pull request description:
~~This PR is based on #22787 ("refactor: actual immutable pointing"), which should be reviewed first.~~ (merged by now)
It aims to make the CWallet shared pointers actually immutable also for the `dumpprivkey` and `dumpwallet` RPC methods. For doing that, some more preparations are needed; we need a const-counterpart to the helper `EnsureLegacyScriptPubKeyMan` that accepts a const CWallet pointer and accordingly also returns a const `LegacyScriptPubKeyMan` instance. The metadata lookup in `dumpwallet` is changed to not need a mutable `ScriptPubKeyMan` instance by avoiding using the `operator[]` in its mapKeyMetadata map, which also avoids repeated lookups.
ACKs for top commit:
laanwj:
Code review ACK d150fe3ad5
Tree-SHA512: 90ac05e21f40c6d0ebb479a71c545da2fd89940b9ca3409d9f932abc983bf8830d34c45086e68880d0d1e994846fbefee7534eec142ff4268e0fa28a1a411d36
22b44fc696 p2p: improve checkaddrman logging with duration in milliseconds (Jon Atack)
ec65bed00e log, timer: add LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE macro (Jon Atack)
325da75a53 log, timer: allow not repeating log message on completion (Jon Atack)
Pull request description:
This patch:
- updates the `logging/timer.h::Timer` class to allow not repeating the log message on completion
- adds a `LOG_TIME_MILLIS_WITH_CATEGORY_MSG_ONCE` macro that prints the descriptive message when logging the start but not when logging the completion
- updates the checkaddrman logging to log the duration, and renames the function like the `-checkaddrman` configuration option in order to prefix every log message with `CheckAddrman` instead of the longer, less pleasant, and different-from-checkaddrman `ForceCheckAddrman` (the Doxygen documentation on the function already makes clear that it is unaffected by `m_consistency_check_ratio`).
before
```
2021-09-21T18:42:50Z [opencon] Addrman checks started: new 64864, tried 1690, total 66554
2021-09-21T18:42:50Z [opencon] Addrman checks completed successfully
```
after
```
2021-09-21T18:42:50Z [opencon] CheckAddrman: new 64864, tried 1690, total 66554 started
2021-09-21T18:42:50Z [opencon] CheckAddrman: completed (76.21ms)
```
To test, build and run bitcoind with `-debug=addrman -checkaddrman=<n>` for a value of `n` in the range of, say, 10 to 40.
ACKs for top commit:
laanwj:
Code review ACK 22b44fc696
Tree-SHA512: 658c0dfaaa9d07092e2418f2d05007c58cc35be6593f22b3c592ce793334a885dd92dacc46bdeddc9d37939cf11174660a094c07c0fa117fbb282953aa45a94d
0fdb619aaf [validation] Always call mempool.check() after processing a new transaction (John Newbery)
2c64270bbe [refactor] Don't call AcceptToMemoryPool() from outside validation.cpp (John Newbery)
92a3aeecf6 [validation] Add CChainState::ProcessTransaction() (John Newbery)
36167faea9 [logging/documentation] Remove reference to AcceptToMemoryPool from error string (John Newbery)
4c24142b1e [validation] Remove comment about AcceptToMemoryPool() (John Newbery)
5759fd12b8 [test] Don't set bypass_limits to true in txvalidation_tests.cpp (John Newbery)
497c9e2964 [test] Don't set bypass_limits to true in txvalidationcache_tests.cpp (John Newbery)
Pull request description:
Similarly to how #18698 added `ProcessNewBlock()` and `ProcessNewBlockHeaders()` methods to the `ChainstateManager` class, this PR adds a new `ProcessTransaction()` method. Code outside validation no longer calls `AcceptToMemoryPool()` directly, but calls through the higher-level `ProcessTransaction()` method. Advantages:
- The interface is simplified. Calling code no longer needs to know about the active chainstate or mempool object, since `AcceptToMemoryPool()` can only ever be called for the active chainstate, and that chainstate knows which mempool it's using. We can also remove the `bypass_limits` argument, since that can only be used internally in validation.
- responsibility for calling `CTxMemPool::check()` is removed from the callers, and run automatically by `ChainstateManager` every time `ProcessTransaction()` is called.
ACKs for top commit:
lsilva01:
tACK 0fdb619 on Ubuntu 20.04
theStack:
Code-review ACK 0fdb619aaf
ryanofsky:
Code review ACK 0fdb619aaf. Only changes since last review: splitting & joining commits, adding more explanations to commit messages, tweaking MEMPOOL_ERROR string, fixing up argument name comments.
Tree-SHA512: 0b395c2e3ef242f0d41d47174b1646b0a73aeece38f1fe29349837e6fb832f4bf8d57e1a1eaed82a97c635cfd59015a7e07f824e0d7c00b2bee4144e80608172
aa1a4c9204 Add file validation to savemempool RPC test (lsilva01)
871e64d22f Add filename to savemempool RPC result (lsilva01)
Pull request description:
Currently, if the user calls the `savemempool` RPC method, there is no way to know
where the file was created (unless the user knows internal implementation details).
This PR adds a return message stating the file name and path where the mempool was saved and changes `mempool_persist.py` to validate this new return message.
ACKs for top commit:
laanwj:
Code review ACK aa1a4c9204
Tree-SHA512: e8b1dd0a8976e5eb15f7476c9651e492d2c621a67e0b726721fa7a2ae0ddd272ee28b87a2d0c650bd635e07fa96bdefe77bece4deb6486ef3ee9a4f83423a840
037c9ee79b fix `XOnlyPubKey::IsFullyValid` comment reference (Sebastian Falbesoner)
Pull request description:
The method name `CreatePayToContract` doesn't exist, very likely it was a (local) working title that was renamed to `CreateTapTweak` later.
ACKs for top commit:
michaelfolkson:
ACK 037c9ee79b
sipa:
ACK 037c9ee79b
Tree-SHA512: ab2b6ca93b66aba83c725b62f39e9f9316f3bea6f75ef35f66a2ac18b22a0a69ff2069cadab0365b28b0af2d30ad5ee3d3022194ac2024a1cdbb81d106fca0bb
14cd7bf793 [test] call CheckPackage for package sanitization checks (glozow)
6876378365 MOVEONLY: move package unit tests to their own file (glozow)
c9b1439ca9 MOVEONLY: mempool checks to their own functions (glozow)
9e910d8152 scripted-diff: clean up MemPoolAccept aliases (glozow)
fd92b0c398 document workspace members (glozow)
3d3e4598b6 [validation] cache iterators to mempool conflicts (glozow)
36a8441912 [validation/rpc] cache + use vsize calculated in PreChecks (glozow)
8fa2936b34 [validation] re-introduce bool for whether a transaction is RBF (glozow)
cbb3598b5c [validation/refactor] store precomputed txdata in workspace (glozow)
0a79eaba72 [validation] case-based constructors for ATMPArgs (glozow)
Pull request description:
This contains the refactors and moves within #22674. There are no behavior changes, so it should be simpler to review.
ACKs for top commit:
ariard:
Code Review ACK 14cd7bf
jnewbery:
Code review ACK 14cd7bf793
laanwj:
Code review ACK 14cd7bf793, thanks for adding documentation and clarifying the code
t-bast:
Code Review ACK 14cd7bf793
Tree-SHA512: 580ed48b43713a3f9d81cd9b573ef6ac44efe5df2fc7b7b7036c232b52952b04bf5ea92940cf73739f4fbd54ecf980cef58032e8a2efe05229ad0b3c639de8a0
79fd28cacb Adds verification step to Schnorr and ECDSA signing (amadeuszpawlik)
Pull request description:
As detailed in #22435, BIP340 defines that during Schnorr signing a verification should be done. This is so that potentially corrupt signage does not leak information about private keys used during the process. This is not followed today as no such verification step is being done. The same is valid for ECDSA signing functions `Sign` and `SignCompact`.
This PR adds this missing verification step to `SignSchnorr`, `Sign` and `SignCompact`.
ACKs for top commit:
sipa:
utACK 79fd28cacb
laanwj:
Code review ACK 79fd28cacb
theStack:
re-ACK 79fd28cacb
Tree-SHA512: 8fefa26caea577ae8631cc16c4e2f4cc6cfa1c7cf51d45a4a34165636ee290950617a17a19b4237c6f7a841db0e40fd5c36ad12ef43da82507c0e9fb9375ab82
bd9c6ade46 wallet: Fixed Grammatical error in bdb.h (zealsham)
Pull request description:
A comment in bdb.h file in the wallet directory contains a grammatical error that makes the underlying code harder to reason about .
The comment which says `/** Indicate the a new database user has began using the database. */` should actually be
`/** indicate that a new database user has began using the database. */`. The former is quite confusing , and leaves you wondering what "the a new database user " is refering to . This pull request thus provides value to the bitcoin codebase by improving readability .
Top commit has no ACKs.
Tree-SHA512: db07cda39f89ac344be3497c884a8c6e4b48276afae18e931a9a8e5732c58eed20645ccd18d6b1212c763f64b1300477c1de26a00b5b3b24e6141ffae3658ca7
The method name `CreatePayToContract` doesn't exist, very likely it was
a (local) working title that was renamed to `CreateTapTweak` later.
Also mention `CheckTapTweak`.
420695c193 contrib: recognize CJDNS seeds as such (Vasil Dimov)
f9c28330a0 net: take the first 4 random bits from CJDNS addresses in GetGroup() (Vasil Dimov)
29ff79c0a2 net: relay CJDNS addresses even if we are not connected to CJDNS (Vasil Dimov)
d96f8d304c net: don't skip CJDNS from GetNetworkNames() (Vasil Dimov)
c2d751abba net: take CJDNS into account in CNetAddr::GetReachabilityFrom() (Vasil Dimov)
9b43b3b257 test: extend feature_proxy.py to test CJDNS (Vasil Dimov)
508eb258fd test: remove default argument of feature_proxy.py:node_test() (Vasil Dimov)
6387f397b3 net: recognize CJDNS addresses as such (Vasil Dimov)
e6890fcb44 net: don't skip CJDNS from GetNetworksInfo() (Vasil Dimov)
e9d90d3c11 net: introduce a new config option to enable CJDNS (Vasil Dimov)
78f456c576 net: recognize CJDNS from ParseNetwork() (Vasil Dimov)
de01e312b3 net: use -proxy for connecting to the CJDNS network (Vasil Dimov)
aedd02ef27 net: make it possible to connect to CJDNS addresses (Vasil Dimov)
Pull request description:
CJDNS overview
=====
CJDNS is like a distributed, shared VPN with multiple entry points where every participant can reach any other participant. All participants use addresses from the `fc00::/8` network (reserved IPv6 range). Installation and configuration is done outside of applications, similarly to VPN (either in the host/OS or on the network router).
Motivation
=====
Even without this PR it is possible to connect two Bitcoin Core nodes through CJDNS manually by using e.g. `-addnode` in environments where CJDNS is set up. However, this PR is necessary for address relay to work properly and automatic connections to be made to CJDNS peers. I.e. to make CJDNS a first class citizen network like IPv4, IPv6, Tor and I2P.
Considerations
=====
An address from the `fc00::/8` network, could mean two things:
1. Part of a local network, as defined in RFC 4193. Like `10.0.0.0/8`. Bitcoin Core could be running on a machine with such address and have peers with those (e.g. in a local network), but those addresses are not relayed to other peers because they are not globally routable on the internet.
2. Part of the CJDNS network. This is like Tor or I2P - if we have connectivity to that network then we could reach such peers and we do relay them to other peers.
So, Bitcoin Core needs to be able to tell which one is it when it encounters a bare `fc00::/8` address, e.g. from `-externalip=` or by looking up the machine's own addresses. Thus a new config option is introduced `-cjdnsreacable`:
* `-cjdnsreacable=0`: it is assumed a `fc00::/8` address is a private IPv6 (1.)
* `-cjdnsreacable=1`: it is assumed a `fc00::/8` address is a CJDNS one (2.)
After setting up CJDNS outside of Bitcoin Core, a node operator only needs to enable this option.
Addresses from P2P relay/gossip don't need that because they are properly tagged as IPv6 or as CJDNS.
For testing
=====
```
[fc32:17ea:e415:c3bf:9808:149d:b5a2:c9aa]:8333
[fc68:7026:cb27:b014:5910:e609:dcdb:22a2]:8333
[fcb3:dc50:e1ae:7998:7dc0:7fa6:4582:8e46]:8333
[fcc7:be49:ccd1:dc91:3125:f0da:457d:8ce]:8333
[fcf2:d9e:3a25:4eef:8f84:251b:1b4d:c596]:8333
```
ACKs for top commit:
dunxen:
ACK 420695c
jonatack:
re-ACK 420695c193 per `git range-diff 23ae793 4fbff39 420695c`
laanwj:
Code review ACK 420695c193
Tree-SHA512: 21559886271aa84671d52b120fa3fa5a50fdcf0fcb26e5b32049c56fab0d606438d19dd366a9c8ce612d3894237ae6d552ead3338b326487e3534399b88a317a
fa93ef5a8a refactor: Take Span in SetSeed (MarcoFalke)
Pull request description:
This makes calling code less verbose and less fragile. Also, by adding
the CKey::data() member function, it is now possible to call HexStr()
with a CKey object.
ACKs for top commit:
sipa:
utACK fa93ef5a8a
laanwj:
Code review ACK fa93ef5a8a
theStack:
Code-review ACK fa93ef5a8a
Tree-SHA512: 73fb999320719ad4b9ab5544018a7a083d140545c2807ee3582ecf7f441040a30b5157e85790b6b840af82f002a7faf30bd8162ebba5caaf2067391c43dc7e25
a04350b86c Open streams_test_tmp file in temporary folder (Martin Leitner-Ankerl)
Pull request description:
The tests `streams_tests/streams_buffered_file` and `streams_tests/streams_buffered_file_rand`
did not use a the temporary directory provided by `BasicTestingSetup`, so it was not possible
to execute multiple of them in parallel. This fixes that.
To reproduce, run
```sh
parallel --halt now,fail=1 './src/test/test_bitcoin --run_test=streams_tests/streams_buffered_file_rand' -- ::: {1..1000}
```
This executes the test 1000 times, one job per CPU. It works on that commit but mergebase fails quickly.
ACKs for top commit:
theStack:
Tested ACK a04350b86c
Tree-SHA512: 705b127e99a0fdbf33362283cc94036ac3aeff364a5e75b0f0a2490114bbaa44cf310ac13d43c254a4b51b72d4e856e9ee584904ce56b2f496b92d995ac7dc24
11115169a1 ci: Build fuzz with libsqlite3-dev (MarcoFalke)
fa7c6efca6 fuzz: Add wallet fuzz test (MarcoFalke)
fa59d2ce5b refactor: Use local args instead of global gArgs in CWallet::Create (MarcoFalke)
fadb44606f build: Inline FUZZ_SUITE_LDFLAGS_COMMON (MarcoFalke)
Pull request description:
Initial sketch to fuzz descriptor wallets. Can be improved in the future.
ACKs for top commit:
mjdietzx:
Code review ACK 1111516
Tree-SHA512: b1d2f24504d1ed5f3c6a031210f04c27c13d4e15576c4acbf50ded37ac45f7b7a5c7553e91d81d4a06e9ea73b3d745a552218d3ef3b2932fa5325a8331b0d3fd
Makes the test more minimal. We're just trying to test that our package
sanitization logic is correct. Now that this code lives in its own
function (rather than inside of AcceptMultipleTransactions), there's no
need to call ProcessNewPackage to test this.
No change in behavior, because package transactions would not be going
through the rbf logic in PreChecks anyway (BIP125 is currently disabled
for package acceptance, see ATMPArgs).
We draw the line here because each individual transaction in package
validation still goes through all PreChecks. For example, checking that
one's own conflicts and dependencies are disjoint (a consensus check)
and individual transaction mempool ancestor/descendant limits.
The aliases are leftover from a previous MOVEONLY refactor - they are
unnecessary and removing them reduces the diff for splitting out mempool
Checks from PreChecks, making RBF variables MemPoolAccept-wide, etc.
-BEGIN VERIFY SCRIPT-
unalias() { sed -i "s:\<$1\>:$2:g" src/validation.cpp; sed -i "/$2 = $2/d" src/validation.cpp; }
unalias nModifiedFees ws.m_modified_fees
unalias nConflictingFees ws.m_conflicting_fees
unalias nConflictingSize ws.m_conflicting_size
unalias setConflicts ws.m_conflicts
unalias allConflicting ws.m_all_conflicting
unalias setAncestors ws.m_ancestors
-END VERIFY SCRIPT-
The tests `streams_tests/streams_buffered_file` and `streams_tests/streams_buffered_file_rand`
did not use a the temporary directory provided by `BasicTestingSetup`, so it was not possible
to execute multiple of them in parallel. This fixes that.
To reproduce, run
```sh
parallel --halt now,fail=1 './src/test/test_bitcoin --run_test=streams_tests/streams_buffered_file_rand' -- ::: {1..1000}
```
This executes the test 1000 times, one job per CPU. It works on that commit but mergebase fails quickly.
80dc829be7 tests: Calculate fees more similarly to CFeeRate::GetFee (Andrew Chow)
ce2cc44afd tests: Test for assertion when feerate is rounded down (Andrew Chow)
0fbaef9676 fees: Always round up fee calculated from a feerate (Andrew Chow)
Pull request description:
When calculating the fee for a feerate, it is possible that the final calculation will have fractional satoshis. Currently those are ignored via truncation which results in the absolute fee being rounded down. Rounding down is problematic because it results in a feerate that is slightly lower than the feerate represented by the `CFeeRate` object. A slightly lower feerate particularly causes issues for coin selection as it can trigger an assertion error. To avoid potentially underpaying the feerate (and the assertion), always round up the calculated fee.
A test is added for the assertion, along with a comment explaining what happens.
It is unlikely that a user can trigger this as it requires a very specific set of rounding errors to occur as well as the transaction not needing any change and being right on the lower bound of the exact match window. However I was able to trigger the assertion while running coin selection simulations, albeit after thousands of transactions and with some weird feerates.
ACKs for top commit:
ryanofsky:
Code review ACK 80dc829be7
promag:
Tested ACK 80dc829be7.
lsilva01:
tACK 80dc829
meshcollider:
utACK 80dc829be7
Tree-SHA512: fe26684c60f236cab48ea6a4600c141ce766dbe59504ec77595dcbd7fd0b34559acc617007f4f499c9155d8fda0a336954413410ba862b19c765c0cfac79d642
CTxMemPool::check() will carry out internal consistency checks 1/n times,
where n is set by the `-checkmempool` configuration option. By default,
mempool consistency checks are disabled entirely on mainnet.
Therefore, this change has no effect on mainnet nodes running with
default configuration. It simply removes the responsibility to trigger
mempool consistency checks from net_processing.
This just calls through to AcceptToMemoryPool() internally, and is currently unused.
Also add a new transaction validation failure reason TX_NO_MEMPOOL to
indicate that there is no mempool.
User-facing error messages should not leak internal implementation
details like function names. Update the MEMPOOL_REJECTED error string
from "Transaction rejected by AcceptToMemoryPool" to the more generic
"Transaction rejected by mempool". Also update the MEMPOOL_ERROR error
message from "AcceptToMemoryPool failed" to the more precise "Mempool
internal error" since this error indicates and internal (e.g.
logic/hardware/etc) failure, and not a transaction rejection.
"This logic is not necessary for memory pool transactions, as
AcceptToMemoryPool already refuses previously-known transaction ids
entirely." refers to the logic at
a206b0ea12/src/main.cpp (L484-L486),
which was later removed in commit 450cbb0944.
CJDNS addresses start with constant 8 bits, so in order to account for
the first 4 random ones, we must take the first 12. Otherwise the entire
CJDNS network will belong to one group.
In some cases addresses come from an external source as a string or as a
`struct sockaddr_in6`, without a tag to tell whether it is a private
IPv6 or a CJDNS address. In those cases interpret the address as a CJDNS
address instead of an IPv6 address if `-cjdnsreachable` is set and the
seemingly-IPv6-address belongs to `fc00::/8`. Those external sources are:
* `-externalip=`
* `-bind=`
* UPnP
* `getifaddrs(3)` (called through `-discover`)
* `addnode`
* `connect`
* incoming connections (returned by `accept(2)`)
CJDNS is set up in the host OS, outside of the application. When the
routing is configured properly then connecting to fc00::/8 results in
connecting to the CJDNS network.
Introduce an option so that Bitcoin Core knows whether this is the case.
Connecting to CJDNS addresses works without a proxy, just like
connecting to an IPv6 address. Thus adapt `CService::GetSockAddr()` to
retrieve the `struct sockaddr*` even for `CService::IsCJDNS()` objects.
AcceptToMemoryPool() is called for an invalid coinbase transaction, so
setting bypass_limits to true or false has no impact on the test.
The only way that changing bypass_limits from true to false could change
the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY).
Since the ATMP call in this test results in INVALID(TX_CONSENSUS) both
before and after this change, there is no change in behavior.
AcceptToMemoryPool() is called for transactions with fees above
minRelayTxFee and with the mempool not full, so setting bypass_limits to
true or false has no impact on the test.
The only way that changing bypass_limits from true to false could change
the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY).
Since all the ATMP calls in this test result in VALID both before and
after this change, there is no change in behavior.
65aaf9495d refactor: move `update_*` structs from txmempool.h to .cpp file (Sebastian Falbesoner)
9947ce6262 refactor: use const reference for parents in `CTxMemPool::UpdateAncestorsOf` (Sebastian Falbesoner)
Pull request description:
These helpers are exclusively used in txmempool.cpp, hence they should also be moved there. The PR also contains a commit which fixes const-correctness for parents in `CTxMemPool::UpdateAncestorsOf` and declares them as reference to avoid a copy.
ACKs for top commit:
promag:
Code review ACK 65aaf9495d. Verified move-only commit locally.
Tree-SHA512: 7ce29f3ba0e68b5355001f27725b00f6d54cc993015356eb40b61b8cdd17db49b980f4c3d798c8e0c940d245dc3a72c474bb9ff3c0ee971ead450786076812c2
6ae9f1cf96 Disable lock contention logging in checkqueue_tests (Jon Atack)
Pull request description:
This patch disables lock contention logging in the checkqueue_tests as some of these tests are designed to be heavily contested to trigger race conditions or other issues. This created very large log files when run with DEBUG_LOCKCONTENTION defined (up to v22) or with lock logging enabled by default in current master.
Examples running the following command:
```
$ ./src/test/test_bitcoin -t checkqueue_tests/test_CheckQueue_Correct_Random -- DEBUG_LOG_OUT > testlog.txt
-rw-r--r-- 87042178 Oct 8 12:41 testlog-with-DEBUG_LOCKCONTENTION-at-v22-run1.txt
-rw-r--r-- 73879896 Oct 8 12:42 testlog-with-DEBUG_LOCKCONTENTION-at-v22-run2.txt
-rw-r--r-- 65150518 Oct 8 12:51 testlog-with-DEBUG_LOCKCONTENTION-at-bb9f76a-run1.txt
-rw-r--r-- 65774554 Oct 8 12:52 testlog-with-DEBUG_LOCKCONTENTION-at-bb9f76a-run2.txt
-rw-r--r-- 73493309 Oct 8 13:00 testlog-current-master-at-991753e-run1.txt
-rw-r--r-- 65616977 Oct 8 13:01 testlog-current-master-at-991753e-run2.txt
-rw-r--r-- 5093 Oct 8 13:04 testlog-with-this-commit-run1.txt
-rw-r--r-- 5093 Oct 8 13:05 testlog-with-this-commit-run2.txt
```
Resolves#23167.
ACKs for top commit:
vasild:
ACK 6ae9f1cf96
Tree-SHA512: b16812ed60c58a1cf40c04ebeca9197ac076b2415f71673ac7bb5b7960a1ff80ba2c909345ad221c7689b0562d17f63a32a629f5d6dbcf0e57130bf5760388c1
As defined in BIP340, a verification step should be executed after
`secp256k1_schnorrsig_sign` to ensure that a potentially corrupted
signature isn't used; using corrupted signatures could reveal
information about the private key used. This applies to ECSDA as
well.
Additionally clears schnorr signature if signing failed.
f3e451bebf [net] Replace GetID() with id in TransportDeserializer constructor (Troy Giorshev)
8c96008ab1 [net] Don't return an optional from TransportDeserializer::GetMessage() (Troy Giorshev)
Pull request description:
Also, access mapRecvBytesPerMsgCmd with `at()` not `find()`. This
throws an error if COMMAND_OTHER doesn't exist, which should never
happen. `find()` instead just accessed the last element, which could make
debugging more difficult.
Resolves review comments from PR19107:
- https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478718436
- https://github.com/bitcoin/bitcoin/pull/19107#discussion_r478714497
ACKs for top commit:
theStack:
Code-review ACK f3e451bebf
ryanofsky:
Code review ACK f3e451bebf. Changes since last review in https://github.com/bitcoin/bitcoin/pull/20364#pullrequestreview-534369904 were simplifying by dropping the third commit, rebasing, and cleaning up some style & comments in the first commit.
Tree-SHA512: 37de4b25646116e45eba50206e82ed215b0d9942d4847a172c104da4ed76ea4cee29a6fb119f3c34106a9b384263c576cb8671d452965a468f358d4a3fa3c003
68018e4c3e test: Avoid excessive locking of `cs_wallet` (Hennadii Stepanov)
7986faf2e0 test: Fix segfault in the psbt_wallet_tests/psbt_updater_test (Hennadii Stepanov)
Pull request description:
The dcd6eeb64a commit (bitcoin/bitcoin#23288) introduced an intermittent failure in the `psbt_wallet_tests/psbt_updater_test` unit test. See bitcoin/bitcoin#23368.
The test failure can be easily made reproducible with the following patch:
```diff
--- a/src/scheduler.cpp
+++ b/src/scheduler.cpp
@@ -57,6 +57,8 @@ void CScheduler::serviceQueue()
Function f = taskQueue.begin()->second;
taskQueue.erase(taskQueue.begin());
+ UninterruptibleSleep(100ms);
+
{
// Unlock before calling f, so it can reschedule itself or another task
// without deadlocking:
```
This PR implements an idea which was mentioned in the [comment](https://github.com/bitcoin/bitcoin/issues/23368#issuecomment-953796339):
> Yes, as I said before this looks like a race where the wallet is deleted before stopping the scheduler: [#23368 (comment)](https://github.com/bitcoin/bitcoin/issues/23368#issuecomment-952808824)
>
> IIRC, the order should be:
>
> * stop scheduler
>
> * delete wallet
>
> * delete scheduler
The second commit introduces a refactoring with no behavior change.
Fixesbitcoin/bitcoin#23368.
ACKs for top commit:
mjdietzx:
Code review ACK 68018e4c3e
Tree-SHA512: d9103f6252aab807453628159dec243bc543a2595eecaa04ec761dca3c2370085592c55d6f50967d69a4ac6e8b5827eec30dd9b025132c99b0bb9aa5911ad915
This makes calling code less verbose and less fragile. Also, by adding
the CKey::data() member function, it is now possible to call HexStr()
with a CKey object.
This should make it easier for the fuzz engine to explore multisig code
paths. See discussion in https://github.com/bitcoin/bitcoin/issues/23105
The downside is that all fuzz inputs that use ConsumeScript are now
invalidated and need to be re-generated.
Another downside may be that most multisig scripts from ConsumeScript are
using likely not fully valid pubkeys.
c5d7e34bd9 scripted-diff: disable unimplemented ArgsManager BOOL/INT/STRING flags (Russell Yanofsky)
b8c069b7a9 refactor: Add explicit DISALLOW_NEGATION ArgsManager flag to clarify flag usage (Russell Yanofsky)
26a50ab322 refactor: Split InterpretOption into Interpret{Key,Value} functions (Russell Yanofsky)
Pull request description:
This is preparation for #16545 or another PR implementing type validation for ArgsManager settings. It fixes misleading usages of existing flags, prevents flags from being similarly misused in the future, and allows validation logic to be added without breaking backwards compatibility.
---
Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation, so current uses of these flags are misleading and will also break backwards compatibility whenever these flags are implemented in a future PR (draft PR is #16545).
An additional complication is that while these flags don't do any real settings validation, they do affect whether setting negation syntax is allowed.
Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is done in three commits, with the first commit cleaning up some code, the second commit adding the DISALLOW_NEGATION flag, and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.
None of the changes affect behavior in any way.
ACKs for top commit:
ajtowns:
utACK c5d7e34bd9
promag:
Code review ACK c5d7e34bd9, which as the new argument `-legacy`.
Tree-SHA512: cad0e06361e8cc584eb07b0a1f8b469e3beea18abb458c4e43d9d16e9f301b12ebf1d1d426a407fbd96f99724ad6c0eae5be05c713881da7c55e0e08044674eb
61ec0539b2 [MOVEONLY] reorder functions in addrman_impl.h and addrman.cpp (John Newbery)
2095df7b7b [addrman] Add Add_() inner function, fix Add() return semantics (John Newbery)
2658eb6d68 [addrman] Rename Add_() to AddSingle() (John Newbery)
e58598e833 [addrman] Add doxygen comment to AddrMan::Add() (John Newbery)
Pull request description:
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.
Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.
ACKs for top commit:
naumenkogs:
ACK 61ec0539b2
mzumsande:
ACK 61ec0539b2
shaavan:
ACK 61ec0539b2
Tree-SHA512: 276f1e8297d4b6d411d05d06ffc7c176f6290a784da039926ab6c471a8ed8e9159ab4f56c893b1285737ae292954930f0d28012d89dfb3f2f825d7df41016feb
9ba7c44265 refactor: get wallet path relative to wallet_dir (Michael Dietz)
Pull request description:
Now that boost has been updated > 1.60 (see #22320), we can simplify how we get
wallet path relative to wallet_dir by using:
`boost::filesystem::lexically_relative`, removing a TODO.
Test coverage comes from `test/functional/wallet_multiwallet.py`
I first tried this in #20265 which was my first attempted PR, and funny enough exactly 1 year later I'm opening this one to hopefully finally close this.
ACKs for top commit:
ryanofsky:
Code review ACK 9ba7c44265. Basically this same code change is made in #20744 commit b70c84348ac7a8e427a1183f894c73e52c734529, so this PR helps simplify that one
lsilva01:
Code Review ACK 9ba7c44
Tree-SHA512: 6ccb91a18bcb52c3ae0c789a94a18fb5be7db7769fd1121552d63f259fbd32b50c3dcf169cec0b02f978321db3bc60eb4b881b8327e9764f32e700236e0d8a35
Now that boost has been updated > 1.60, we can simplify how we get
wallet path relative to wallet_dir by using:
`boost::filesystem::lexically_relative`
d891ae7681 Introduce new V4 format addrman (Pieter Wuille)
Pull request description:
#23306 effectively changed the on-disk format in an incompatible way: old deserializers cannot deal with multiple entries for the same IP.
Introduce a `V4_MULTIPORT` format, and increment the compatibility base, so that old versions correctly recognize it as an incompatible future version, rather than corruption.
ACKs for top commit:
naumenkogs:
ACK d891ae7681
ajtowns:
utACK d891ae7681
vasild:
ACK d891ae7681
Tree-SHA512: de2153beb59152504ee0656dd0cc0b879b09136eb07e3ce0426d2fea778adfabacebbce5cf1a9a65dc99ad4e99cda42ab26743fe672fb82a9fbfec49c4cccb4d
54011e7aa2 refactor: use CWallet const shared pointers when possible (Karl-Johan Alm)
96461989a2 refactor: const shared_ptrs (Karl-Johan Alm)
Pull request description:
```C++
const std::shared_ptr<CWallet> wallet = x;
```
means we can not do `wallet = y`, but we can totally do `wallet->DestructiveOperation()`, contrary to what that line looks like.
This PR
* introduces a new convention: always use const shared pointers to `CWallet`s (even when we mutate the pointed-to thing)
* uses `const shared_ptr<const CWallet>` everywhere where wallets are not modified
In the future, this should preferably apply to all shared pointers, not limited to just `CWallet`s.
Both of these serve the same purpose: to dispell the misconception that `const shared_ptr<X>` immutates `X`. It doesn't, and it's dangerous to leave this misconception as is, for obvious reasons.
ACKs for top commit:
theStack:
re-ACK 54011e7aa2
Tree-SHA512: 3bf4062fc821751be30770c6b4ead10a016847970f155a0a5156f304347d221b9830840030c2fbfba8cd1e282f4eda45f5b4107fe6df8138afdcb6c2e95a2836
This is not only cleaner but also helps make sure we are always using
the virtual size measure that includes the sigop weight heuristic (which
is the vsize the mempool would return).
This bool was originally part of Workspace and was removed in #22539
when it was no longer needed in Finalize(). Re-introducing it because,
once again, multiple functions will need to know whether we're doing an
RBF. Member of MemPoolAccept so that we can use this to inform package
RBF in the future.
No change in behavior.
ATMPArgs can continue to have granular rules like switching BIP125
on/off while we create an interface for the different sets of rules for
single transactions vs multiple-testmempoolaccept vs package validation.
This is a cleaner interface than manually constructing the args, which
makes it easy to mix up ordering, use the wrong default, etc. It also
means we don't need to edit ATMP/single transaction validation code
every time we update ATMPArgs for package validation.
Previously, Add() would return true if the function created a new
AddressInfo object, even if that object could not be successfully
entered into the new table and was deleted. That would happen if the new
table position was already taken and the existing entry could not be
removed.
Instead, return true if the new AddressInfo object is successfully
entered into the new table. This fixes a bug in the "Added %i addresses"
log, which would not always accurately log how many addresses had been
added.
p2p_addrv2_relay.py and p2p_addr_relay.py need to be updated since they
were incorrectly asserting on the buggy log (assuming that addresses are
added to addrman, when there could in fact be new table position
collisions that prevent some of those address records from being added).
d5f985e51f multiprocess: Add new bitcoin-gui, bitcoin-qt, bitcoin-wallet init implementations (Russell Yanofsky)
Pull request description:
Add separate `interfaces::Init` subclasses for `bitcoin-wallet`, `bitcoin-gui`, and `bitcoin-qt` binaries instead of sharing `bitcoind` and `bitcoin-node` init subclasses in different binaries. After this, the new init subclasses can be customized in #10102, so node and wallet code is dropped from the `bitcoin-gui` binary and wallet code is dropped from into the `bitcoin-node` binary.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
lsilva01:
reACK d5f985e
hebasto:
re-ACK d5f985e51f, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/23006#pullrequestreview-787537444) review.
Tree-SHA512: 6784210bd9ce3a6fbc66852680d0e9bc513c072dc538aeb7f48cb6a41580d3f567ccef04f975ee767a714a4b05d4d87273e94a79abda1b9c25d5ac4bbe752006
92617b7a75 effectively changed the
on-disk format in an incompatible way: old deserializers cannot
deal with multiple entries for the same IP.
Introduce a V4_MULTIPORT format, and increment the compatibility base,
so that old versions correctly recognize it as an incompatible future
version.
92617b7a75 Make AddrMan support multiple ports per IP (Pieter Wuille)
Pull request description:
For a long part of Bitcoin's history, this codebase has aggressively avoided making automatic connections to anything but nodes running on port 8333. I'd like to propose changing that, and this is a first PR necessary for that.
The folklore justification (eventually actually added as a comment to the codebase in #20668) is that this is to prevent the Bitcoin P2P network from being leveraged to perform a DoS attack on other services, if their IP/port would get rumoured. It appears, at least the current network scale - and probably significantly larger - that the impact is very low at best (see calculations by vasild in https://github.com/bitcoin/bitcoin/issues/5150#issuecomment-853888909 e.g.). Another possible justification would be a risk that treating different IP:port combinations separately would help perform Eclipse attacks (by an attacker rumouring their own IP with many ports). This concern is (a) no different than what is possible with IPv6 (where large ranges of IP addresses are very cheaply available), and (b) already hopefully sufficiently addressed by addrman's design (which limits access through based selected based on network groups).
And this policy has downsides too; in particular, a fixed port is easy to detect, and a very obvious sign a Bitcoin node is running there.
One obstacle in moving away from a default port that is the fact that addrman is currently restricted to a single entry per IP address. If ports are no longer expected to be generally always the default one, we need to deal with the case where conflicting information is relayed. It turns out there is a very natural solution to this: treat (IP,port) combination exactly as we're treating IPs now; this automatically means that the same IP may appear with multiple ports, simply because those would be distinct entries. Given that indexing into addrman's bucket _already_ uses the port number, the only change required is making all addrman lookup be (IP,port) (aka `CService`) based, rather than IP (aka `CNetAddr`) based.
This PR doesn't include any change to the actual outbound connection preference logic, as perhaps that's something that we want to phase in more gradually.
ACKs for top commit:
jnewbery:
Code review ACK 92617b7a75
naumenkogs:
ACK 92617b7a75
ajtowns:
ACK 92617b7a75
vasild:
ACK 92617b7a75
Tree-SHA512: 9eef06ce97a8b54a3f05fb8acf6941f253a9a5e0be8ce383dd05c44bb567cea243b74ee5667178e7497f6df2db93adab97ac66edbc37c883fd8ec840ee69a33f
This commit does not change behavior in any way. See previous commit for
complete rationale, but these flags are being disabled because they
aren't implemented and will otherwise break backwards compatibility when
they are implemented.
-BEGIN VERIFY SCRIPT-
sed -i 's:\(ALLOW_.*\) \(//!< unimplemented\):// \1\2:' src/util/system.h
sed -i '/DISALLOW_NEGATION.*scripted-diff/d' src/util/system.cpp
git grep -l 'ArgsManager::ALLOW_\(INT\|STRING\)' | xargs sed -i 's/ArgsManager::ALLOW_\(INT\|STRING\)/ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION/g'
git grep -l 'ALLOW_BOOL' -- ':!src/util/system.h' | xargs sed -i 's/ALLOW_BOOL/ALLOW_ANY/g'
-END VERIFY SCRIPT-
Currently, ALLOW_{INT|BOOL|STRING} flags don't do any real validation,
so current uses of these flags are misleading and will also break
backwards compatibility whenever these flags are implemented in a future
PR (draft PR is #16545).
An additional complication is that while these flags don't do any real
settings validation, they do affect whether setting negation syntax is
allowed.
Fix this mess by disabling ALLOW_{INT|BOOL|STRING} flags until they are
implemented, and adding an unambiguous DISALLOW_NEGATION flag. This is
done in two commits, with this commit adding the DISALLOW_NEGATION flag,
and the next commit disabling the ALLOW_{INT|BOOL|STRING} flags.
082c5bf099 [refactor] pass coinsview and height to check() (glozow)
ed6115f1ea [mempool] simplify some check() logic (glozow)
9e8d7ad5d9 [validation/mempool] use Spend/AddCoin instead of UpdateCoins (glozow)
09d18916af MOVEONLY: remove single-use helper func CheckInputsAndUpdateCoins (glozow)
e8639ec26a [mempool] remove now-unnecessary code (glozow)
54c6f3c1da [mempool] speed up check() by using coins cache and iterating in topo order (glozow)
30e240f65e [bench] Benchmark CTxMemPool::check() (glozow)
cb1407196f [refactor/bench] make mempool_stress bench reusable and parameterizable (glozow)
Pull request description:
Remove the txmempool <-> validation circular dependency by removing txmempool's dependency on validation. There are two functions in txmempool that need validation right now: `check()` and `removeForReorg()`. This PR removes the dependencies in `check()`.
This PR also improves the performance of `CTxMemPool::check()` by walking through the entries exactly once, in ascending ancestorcount order, which guarantees that we see parents before children.
ACKs for top commit:
jnewbery:
reACK 082c5bf099
GeneFerneau:
tACK [082c5bf](082c5bf099)
rajarshimaitra:
tACK 082c5bf099
theStack:
Code-review ACK 082c5bf099
Tree-SHA512: 40ac622af1627b5c3e6abb4f0f035d833265a8c5e8dc88faf5354875dfb5137f137825e54bbd2a2668ed37b145c5d02285f776402629f58596e51853a9a79d29
a52f1d1340 walletdb: Use SQLiteDatabase for mock wallet databases (Andrew Chow)
a78c229808 tests: Place into mapWallet in coinselector_tests (Andrew Chow)
Pull request description:
#23288 changed coinselector_tests to use `DescriptorScriptPubKeyMan`, but it also ended up significantly slowing down the test, from 4 seconds to over 1 minute. It appears that the source of this slow down is with `CWallet::AddToWallet`, and primarily due to writing data to the mock wallet database. Because the only thing that is actually needed is for the created transaction to be placed into `CWallet::mapWallet`, this PR removes the call to `AddToWallet` and just places the transaction into `mapWallet` directly. This reduces the test time to 5 seconds.
To speed things up further, `CreateMockWalletDatabase` is changed to make a `SQLiteDatabase` instead of a `BerkeleyDatabase`. This is safe because there are no tests that require a specific mock database type.
ACKs for top commit:
brunoerg:
tACK a52f1d1340
lsilva01:
tACK a52f1d1. Performed 74.36% better on Ubuntu 20.04 (VM, 12 MB, 8vCPU).
glozow:
utACK a52f1d1340
Tree-SHA512: da77936bfd2e816d2e71703567b9389d0ee79f3a4a690802ffe3469df5bed371b296cb822b897f625654dab9436d91fd6bc02364a518a47d746e487d70a72595
While const shared_ptr<X> gives us an immutable shared pointer to a mutable X (we can't set it to some other X later), shared_ptr<const X> gives us a shared pointer to an immutable X. Importantly, we can recast shared_ptr<X> into shared_ptr<const X>, but not the other way around. We do this for two reasons: because it makes the code safer to guarantee the wallet is not modified, and because it further dispells the misconception that const shared_ptr<X> gives immutability to X.
Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is.
We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.
Default to SQLiteDatabase instead of BerkeleyDatabase for
CreateDummyWalletDatabase. Most tests already use descriptor wallets and
the mock db doesn't really matter for tests. The tests where it does
matter will make the db directly.
Instead of using AddToWallet so that making a COutput will work,
directly add the transaction into wallet.mapWallet. This bypasses many
checks that AddToWallet will do which are pointless and just slow down
this test.
58765a450c qt: Use only Qt translation primitives in GUI code (W. J. van der Laan)
Pull request description:
Use `Object::tr`, `QT_TRANSLATE_NOOP`, and `QCoreApplication::translate` as appropriate instead of using `_()` which doesn't get picked up.
Replaces bitcoin/bitcoin#22764
Edit: I checked that the strings end up in the appropriate context in `bitcoin_en.ts` after `make translate`:
- "Settings file could not be read" "Settings file could not be written" end up in `bitcoin-core`
- "(press q to shutdown and continue later)" and "press q to shutdown" end up in `SplashScreen`
ACKs for top commit:
hebasto:
ACK 58765a450c
Tree-SHA512: d0cc5901426c47d411d0fe75aac195b9684b51385f74c161da772dbf9f0977f7ad7a0976e17366f49b40718e9b6eb87c3e93306dc845f97adddbc47d00731742
fa4ec1c0bd Make GenTxid boolean constructor private (MarcoFalke)
faeb9a5753 remove unused CTxMemPool::info(const uint256& txid) (MarcoFalke)
Pull request description:
This boolean argument is either verbose (when used with a named arg) or unintuitive and dangerous (when used as a plain bool).
Fix that by making the constructor private.
ACKs for top commit:
laanwj:
Code review ACK fa4ec1c0bd
jnewbery:
Code review ACK fa4ec1c0bd
glozow:
code review ACK fa4ec1c0bd
Tree-SHA512: bf08ee09168885cfda71e5a01ec412b93964662a90dd9d91e75f7fdf2eaff7c21a95204d0e90b00438bfeab564d0aea66bdb9c0394ee7a05743e65a817159446
66f6efc70a rpc: improve TransactionDescriptionString() "generated" help (Jon Atack)
296cfa312f test: add listtransactions/listsinceblock "trusted" coverage (Jon Atack)
d95913fc43 rpc: fix "trusted" description in TransactionDescriptionString (Jon Atack)
Pull request description:
The RPC gettransaction, listtransactions, and listsinceblock helps returned by `TransactionDescriptionString()` inform the user that the `trusted` boolean field is only present if the transaction is trusted and safe to spend from.
The field is in fact returned by `WalletTxToJSON()` when the transaction has 0 confirmations (or negative confirmations, if conflicted), and it can be true or false.
This patch fixes the help, adds test coverage, and touches up the help for the neighboring `generate` field.
ACKs for top commit:
rajarshimaitra:
tACK 66f6efc70a
theStack:
Tested ACK 66f6efc70a
Tree-SHA512: 4c2127765b82780e07bbdbf519d27163d414d9f15598e01e02210f210e6009be344c84951d7274e747b1386991d4c3b082cd25aebe885fb8cf0b92d57178f68e
d047ed729f external_signer: improve fingerprint matching logic (stop on first match) (Sebastian Falbesoner)
Pull request description:
The fingerprint matching logic in `ExternalSigner::SignTransaction` currently always iterates all inputs of a PSBT, even after a match has already been found. I guess the reason for that is not that it was not thought of, but rather the fact that breaking out of a nested loop is simply not possible (at least not without adding ugly constructs like gotos or extra state variables).
This PR fixes this by using `std::any_of` from C++'s standard library, see http://www.cplusplus.com/reference/algorithm/any_of/
ACKs for top commit:
lsilva01:
Code Review ACK d047ed729f
Sjors:
utACK d047ed7
Zero-1729:
crACK d047ed729f
mjdietzx:
Code review ACK d047ed729f
hebasto:
ACK d047ed729f, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 447e7c0c6a5b5549a2c09d52e55ba4146302c1a06e4d96de11f6945d09f98c89129cba221202dff7e0718e01a83dd173b9f19b1f02b6be228978f3f6e35d8096
ea4b61a157 refactor: remove references to deprecated values under std::allocator (Pasta)
Pull request description:
Removes usages of allocator::pointer, allocator::const_pointer, allocator::reference and allocator::const_reference which are deprecated in c++17 and **removed** in c++20. See https://en.cppreference.com/w/cpp/memory/allocator
Also prefers `using` over `typedef` see: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rt-using I'll be happy to revert this if requested so
ACKs for top commit:
laanwj:
Re-ACK ea4b61a157
Tree-SHA512: 9353e47a7de27bcd91b341eb2d832318b51fce9f508fcc791f05c02c5a160f430f4e7214e76f4b3e29639750d311c679789d8b7409255b13637391e4575c9ebe
2d2edc1248 tests: Use Descriptor wallets for generic wallet tests (Andrew Chow)
99516285b7 tests: Use legacy change type in subtract fee from outputs test (Andrew Chow)
dcd6eeb64a tests: Use descriptors in psbt_wallet_tests (Andrew Chow)
4b1588c6bd tests: Use DescriptorScriptPubKeyMan in coinselector_tests (Andrew Chow)
811319fea4 tests, gui: Use DescriptorScriptPubKeyMan in GUI tests (Andrew Chow)
9bf0243872 bench: Use DescriptorScriptPubKeyMan for wallet things (Andrew Chow)
5e54aa9b90 bench: remove global testWallet from CoinSelection benchmark (Andrew Chow)
a5595b1320 tests: Remove global vCoins and testWallet from coinselector_tests (Andrew Chow)
Pull request description:
Currently, various tests use `LegacyScriptPubKeyMan` because it was convenient for the refactor that introduced the `ScriptPubKeyMan` interface. However, with the legacy wallet slated to be removed, these tests should not continue to use `LegacyScriptPubKeyMan` as they are not testing any specific legacy wallet behavior. These tests are changed to use `DescriptorScriptPubKeyMan`s.
Some of the coin selection tests and benchmarks had a global `testWallet`, but this seemed to cause some issues with ensuring that descriptors were set up in that wallet for each test. Those have been restructured to not have any global variables that may be modified between tests.
The tests which test specific legacy wallet behavior remain unchanged.
ACKs for top commit:
laanwj:
Code review ACK 2d2edc1248
brunoerg:
tACK 2d2edc1248
Tree-SHA512: 6d60e5978e822d48e46cfc0dae4635fcb1939f21ea9d84eb72e36112e925554b7ee8f932c7ed0c4881b6566c6c19260bec346abdff1956ca9f300b30fb4e2dd1
6911ab95f1 wallet: fix segfault by avoiding invalid default-ctored `external_spk_managers` entry (Sebastian Falbesoner)
Pull request description:
Fixes#23321 (bug reported by Josef Vondrlik (josef-v)).
In the method `CWallet::LoadActiveScriptPubKeyMan`, the map `external_spk_managers` (or `internal_spk_managers`, if parameter `internal` is false) is accessed via std::map::operator[], which means that a default-ctored entry is created with a null-pointer as value, if the key doesn't exist. As soon as this value is dereferenced, a segmentation fault occurs, e.g. in `CWallet::KeypoolCountExternalKeys`.
The bevaviour can be reproduced by the following steps (starting with empty regtest datadir):
```
$ ./src/bitcoind -regtest -daemon
$ ./src/bitcoin-cli -regtest -named createwallet_name=wallet descriptors=true blank=true
$ cat regtest-descriptors.txt
[
{
"desc": "tr([e4445899/49'/1'/0']tprv8ZgxMBicQKsPd8jCeBWsYLEoWxbVgzJDatJ7XkwQ6G3uF4FsHuaziHQ5JZAW4K515nj6kVVwPaNWZSMEcR7aFCwL4tQqTcaoprMKTTtm6Zg/1/*)#mr3llm7f",
"timestamp": 1634652324,
"active": true,
"internal": true,
"range": [
0,
999
],
"next": 0
}
]
$ ./src/bitcoin-cli -regtest importdescriptors "$(cat regtest-descriptors.txt)"
[
{
"success": true
}
]
$ ./src/bitcoin-cli -regtest getwalletinfo
error: timeout on transient error: Could not connect to the server 127.0.0.1:18443 (error code 1 - "EOF reached")
```
ACKs for top commit:
achow101:
Code Review ACK 6911ab95f1
lsilva01:
Tested ACK 6911ab9 on Ubuntu 20.04.
instagibbs:
ACK 6911ab95f1
Tree-SHA512: 76aa96847cf2739413fb68fb902afef0b3ab9381178dd62fb0abac69f853f1f6523d73c60e610375b9a7730f275eda9162503b89f5be6e6e349a8d047b59c8dc
632aad9e6d Make CAddrman::Select_ select buckets, not positions, first (Pieter Wuille)
Pull request description:
The original CAddrMan behaviour (before #5941) was to pick a uniformly random non-empty bucket, and then pick a random element from that bucket. That commit, which introduced deterministic placement of entries in buckets, changed this to picking a uniformly random non-empty bucket position instead.
I believe that was a mistake. Buckets are our best metric for spreading out requests across independently-controlled nodes. That
does mean that if a bucket has fewer entries, its entries are supposed to be picked more frequently.
This PR reverts to the original high-level behavior, but on top of the deterministic placement logic.
ACKs for top commit:
jnewbery:
utACK 632aad9e6d
naumenkogs:
ACK 632aad9e6d
mzumsande:
ACK 632aad9e6d
Tree-SHA512: 60768afba2b6f0abd0dddff04381cab5acf374df48fc0e883ee16dde7cf7fd33056a04b573cff24a1b4d8e2a645bf0f0b3689eec84da4ff330e7b59ef142eca1
9c1052a521 wallet: Default new wallets to descriptor wallets (Andrew Chow)
f19ad40463 rpc, wallet: Descriptor wallets are no longer experimental (Andrew Chow)
Pull request description:
Changes the default wallet type from legacy to descriptors. Descriptor wallets will now by the default type. Additionally, descriptor wallets will no longer be marked as experimental.
This follows the timeline proposed in #20160
ACKs for top commit:
lsilva01:
Tested ACK 9c1052a521 on Ubuntu 20.04
prayank23:
tACK 9c1052a521
meshcollider:
Code review ACK 9c1052a521
Tree-SHA512: 834e6fec88e0c18673af7ebe135bd5333694d1be502164eb93a90e3e76c27974165aa4e59426945100c88e4eca07356e16886ef5b05cf789683ecb23fc71a12a
fa2662c293 net: Avoid logging AlreadyHaveTx when disconnecting misbehaving peer (MarcoFalke)
Pull request description:
There is no need to log `AlreadyHaveTx` for an inv when a peer is marked for disconnection due to sending that inv. In fact, I find it confusing that a `block-relay-only` connection calls `AlreadyHaveTx` at all. Also there is no need to call `AddKnownTx` when the peer is marked for disconnection.
ACKs for top commit:
naumenkogs:
ACK fa2662c293
jnewbery:
Code review ACK fa2662c293
dunxen:
Concept and code review ACK fa2662c293
Tree-SHA512: 9996b807a824021f992b5281d82ff0cbbe6a442c2fedf7dfd6adda64ccc5e0ef4fb0ff91ab75086f975837bbbb7a5934ac7e671a80dcababa7203c92fc0c7f84
4307849256 [mempool] delete exists(uint256) function (glozow)
d50fbd4c5b create explicit GenTxid::{Txid, Wtxid} ctors (glozow)
Pull request description:
We use the same type for txids and wtxids, `uint256`. In places where we want the ability to pass either one, we distinguish them using `GenTxid`.
The (overloaded) `CTxMemPool::exists()` function is defined as follows:
```c
bool exists(const uint256& txid) const { return exists(GenTxid{false, txid}); }
```
It always assumes that a uint256 is a txid, which is a footgunny interface.
Querying by wtxid returns a false negative if the transaction has a witness. 🐛
Another approach would be to try both:
```c
bool exists(const uint256& txid) const { return exists(GenTxid{false, txid}) || exists(GenTxid{false, txid}); }
```
But that's slower and wrongfully placing the burden on the callee; the caller always knows whether the hash is a txid or a wtxid.
ACKs for top commit:
laanwj:
Code review ACK 4307849256
jnewbery:
Tested and code review ACK 4307849256
MarcoFalke:
review ACK 4307849256👘
Tree-SHA512: 8ed167a96f3124b6c14e41073c8358658114ce121a15a4cca2db7a5ac565903a6236e34e88ac03382b8bb8b68e3999abbfc5718bc8c22476554d6b49a5298eec
Follow-up to:
* commit 700c42b85d, which replaced pIndex
with block_hash in AddToWalletIfInvolvingMe.
* commit 9700fcb47f, which replaced
posInBlock with confirm.nIndex.
1946af2c45 Add comment to COIN constant. (Kennan Mell)
Pull request description:
The COIN constant is critical in understanding Bitcoin's supply, but what it represents isn't clear from the name of the constant. Adding a comment clarifies the meaning of the constant for future readers.
ACKs for top commit:
lsilva01:
ACK 1946af2
shaavan:
ACK 1946af2c45
Tree-SHA512: ba27c6bd4a4c92664a71ef081104e9e50e47022d68eb955c247b7c49a0046a42bf64bf23a14563c646411b7be6027fea918cc62baa01bbf319b82d14d368f280
fadf1186c8 p2p: Use mocktime for ping timeout (MarcoFalke)
Pull request description:
It is slightly confusing to use mocktime for some times, but not others.
Start fixing that by making the ping timeout use mocktime.
The only downside would be that tests that use mocktime disconnect peers after this patch. However, I don't think this is an issue, as the inactivity check is already disabled for all functional tests after commit 6d76b57ca0. Only one unit test needed the inactivity check disabled as part of this patch.
A nice side effect of this patch is that the `p2p_ping` functional test now runs 4 seconds faster.
ACKs for top commit:
laanwj:
Code review ACK fadf1186c8
Tree-SHA512: e9e7b21040a89d9d574b3038f85a67e6336de6cd6e41aa286769cd03cada6e75a94ec01700e052e56d822ef85d7813cc06bf7e67b81543eff8917a16cdccf942
be7f4130f9 Fix K1/K2 use in the comments in ChaCha20-Poly1305 AEAD (=)
Pull request description:
As per [#22331](https://github.com/bitcoin/bitcoin/pull/22331) and the [Detailed Construction of the ChaCha20Forward4064-Poly1305@Bitcoin cipher suite](https://gist.github.com/jonasschnelli/c530ea8421b8d0e80c51486325587c52#detailed-construction) mentioned in BIP 324, K1 is used for encrypting the associated data(message length) and instantiating the Poly1305 MAC while K2 is used for encrypting the payload. This PR fixes the comments which need to be updated in:
1. The test vector in `src/test/crypto_tests.cpp`
2. In `src/crypto/chacha_poly_aead.h`, `m_chacha_main` is a K2 ChaCha20 cipher instance and should be used for encrypting the payload. Also, `m_chacha_header` is a K1 ChaCha20 cipher instance and is used for encrypting the length and instantiating the Poly1305 MAC.
ACKs for top commit:
siv2r:
ACK be7f413
jonatack:
ACK be7f4130f9
Zero-1729:
ACK be7f413
shaavan:
reACK be7f4130f9
Tree-SHA512: 9d3d0f45cf95d0a87b9f04c26f04b9ea78b2f2fa578d3722146a79dd0d377b9867532fc62e02b8e1487420df7702a1f033d15db562327535940c2049cbde401f
96f469f91b netinfo: print peer counts for all reachable networks (Jon Atack)
Pull request description:
instead of only for networks we have peer connections to.
Users reported the previous behavior caused confusion, as no column was printed when a network was reachable but no peers were connected. Users expected a column to be printed with 0 peers. This commit aligns behavior with that expectation.
In addition, the ipv4, ipv6, and onion columns were always printed whether or not they were reachable. With this change, only the reachable ones will be returned.
Example with CJDNS reachable but no CJDNS peers (built on #23077 and #23175):
before
```
ipv4 ipv6 onion i2p total block manual
in 0 0 12 5 17
out 8 1 6 4 19 2 8
total 8 1 18 9 36
```
after
```
ipv4 ipv6 onion i2p cjdns total block manual
in 0 0 12 5 0 17
out 8 1 6 4 0 19 2 8
total 8 1 18 9 0 36
```
There is one additional space between the in/out/total row headers and the network counts.
ACKs for top commit:
jsarenik:
Tested ACK 96f469f91
prayank23:
utACK 96f469f91b
laanwj:
Code review and lightly tested ACK 96f469f91b
naumenkogs:
ACK 96f469f91b.
hebasto:
ACK 96f469f91b, tested by comparing the output of `watch src/bitcoin-cli -netinfo` with the Peer tab of the Node window in the GUI 🐅
Tree-SHA512: 3489f40148a2bd0afc9eef1e1577d44150c1fccec8dbf2a675bc23aa9343bfcae6c4039f5b96e54730668c83f40bc932fb6808f5540e86ff7249fde8dc0fff67
In the method `CWallet::LoadActiveScriptPubKeyMan`, the map
`external_spk_managers` (or `internal_spk_managers`, if parameter
`internal` is false) is accessed via std::map::operator[], which means
that a default-ctored entry is created with a null-pointer as value, if
the key doesn't exist. As soon as this value is dereferenced, a
segmentation fault occurs, e.g. in `CWallet::KeypoolCountExternalKeys`.
The bevaviour can be reproduced by the following steps (starting with empty regtest datadir):
$ ./src/bitcoind -regtest -daemon
$ ./src/bitcoin-cli -regtest -named createwallet_name=wallet descriptors=true blank=true
$ cat regtest-descriptors.txt
[
{
"desc": "tr([e4445899/49'/1'/0']tprv8ZgxMBicQKsPd8jCeBWsYLEoWxbVgzJDatJ7XkwQ6G3uF4FsHuaziHQ5JZAW4K515nj6kVVwPaNWZSMEcR7aFCwL4tQqTcaoprMKTTtm6Zg/1/*)#mr3llm7f",
"timestamp": 1634652324,
"active": true,
"internal": true,
"range": [
0,
999
],
"next": 0
}
]
$ ./src/bitcoin-cli -regtest importdescriptors "$(cat regtest-descriptors.txt)"
[
{
"success": true
}
]
$ ./src/bitcoin-cli -regtest getwalletinfo
error: timeout on transient error: Could not connect to the server 127.0.0.1:18443 (error code 1 - "EOF reached")
Bug reported by Josef Vondrlik (josef-v).
fa2d611bed style: Sort (MarcoFalke)
fa1e5de2db scripted-diff: Move bloom to src/common (MarcoFalke)
fac303c504 refactor: Remove unused MakeUCharSpan (MarcoFalke)
Pull request description:
To avoid having all files at the top level `./src` directory, start moving them to their respective sub directory according to https://github.com/bitcoin/bitcoin/issues/15732.
`bloom` currently depends on libconsensus (`CTransaction`, `CScript`, ...) and it is currently located in the libcommon. Thus, move it to `src/common/`. (libutil in `src/util/` is for stuff that doesn't depend on libconsensus).
ACKs for top commit:
theStack:
Code-review ACK fa2d611bed
ryanofsky:
Code review ACK fa2d611bed
fanquake:
ACK fa2d611bed - source shuffle starts now.
Tree-SHA512: d2fbc31b81741e9f0be539e1149542c9ca39958c240e12e8e757d882beccd0f0debdc10dcce146a05f03ef9f5c6247900a461a7a4799b515e8716dfb9af1fde2
