f1c33ee4ac tests: implement BIP341 test vectors (Pieter Wuille)
ac3037df11 tests: BIP341 test vector generation (Pieter Wuille)
ca83ffc2ea tests: add deterministic signing mode to ECDSA (Pieter Wuille)
c98c53f20c tests: abstract out precomputed BIP341 signature hash elements (Pieter Wuille)
a5bde018b4 tests: give feature_taproot access to sighash preimages (Pieter Wuille)
5140825096 tests: add more fields to TaprootInfo (Pieter Wuille)
2478c6730a Make signing follow BIP340 exactly w.r.t. aux randomness (Pieter Wuille)
Pull request description:
This PR adds code to `test/functional/feature_taproot.py` which runs through a (deterministic) scenario covering several aspects of the wallet side of BIP341 (scriptPubKey computation from keys/scripts, control block computation, key path spending), with the ability to output test vectors in mediawiki format based on this scenario. The generated tests are then also included directly in `src/test/script_tests.cpp` and `src/test/script_standard_tests.cpp`.
I intend to add these test vectors to BIP341 itself: https://github.com/bitcoin/bips/pull/1225
ACKs for top commit:
laanwj:
Code review ACK f1c33ee4ac
Tree-SHA512: fcf7109539cb214d3190516b205cd32d2b1b452f14aa66f4107acfaa8bfc7d368f626857f1935665a4342eabc0b9ee8aba608a7c0a2494bec0b498e723439c9d
ad085f9ba1 multiprocess: Delay wallet client construction (Russell Yanofsky)
Pull request description:
Delay wallet client construction until after logging, thread and other init for two reasons:
- More responsive multiprocess GUI startup. When bitcoin-gui is started this moves the call from bitcoin-gui to bitcoin-node that spawns bitcoin-wallet off of the GUI event thread and onto the background GUI init executor thread.
- Avoids feature_logging.py test failures with bitcoin-node by making bitcoin-wallet logging start after bitcoin-node logging starts,
because the tests are not written to handle the bitcoin-wallet logging init code running first.
This partially reverts commit b266b3e0bf, moving wallet client creation back to the place it was located before.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
ACKs for top commit:
laanwj:
code review ACK ad085f9ba1
hebasto:
ACK ad085f9ba1, I have reviewed the code and it looks OK.
Tree-SHA512: 74d957ce2ee096db745c517124f60800185814b06c20db676090e10dce1b90311adbab02865a69731f8c39b9365f9ee14be0830ca1368cac9b474801ea92bad5
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
