This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
31b136e580 Don't declare de facto const reference variables as non-const (practicalswift)
1c65c075ee Don't declare de facto const member functions as non-const (practicalswift)
Pull request description:
_Meta: This is the second and final part of the `const` refactoring series (part one: #20581). **I promise: no more refactoring PRs from me in a while! :)** I'll now go back to focusing on fuzzing/hardening!_
Changes in this PR:
* Don't declare de facto const member functions as non-const
* Don't declare de facto const reference variables as non-const
Awards for finding candidates for the above changes go to:
* `clang-tidy`'s [`readability-make-member-function-const`](https://clang.llvm.org/extra/clang-tidy/checks/readability-make-member-function-const.html) check ([list of `clang-tidy` checks](https://clang.llvm.org/extra/clang-tidy/checks/list.html))
* `cppcheck`'s `constVariable` check ([list of `cppcheck` checks](https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/))
See #18920 for instructions on how to analyse Bitcoin Core using Clang Static Analysis, `clang-tidy` and `cppcheck`.
ACKs for top commit:
ajtowns:
ACK 31b136e580
jonatack:
ACK 31b136e580
theStack:
ACK 31b136e580❄️
Tree-SHA512: f58f8f00744219426874379e9f3e9331132b9b48e954d24f3a85cbb858fdcc98009ed42ef7e7b4619ae8af9fc240a6d8bfc1c438db2e97b0ecd722a80dcfeffe
66d012ad7f test: RPC: getblock fee calculations (Elliott Jin)
bf7d6e31b1 RPC: getblock: tx fee calculation for verbosity 2 via Undo data (Elliott Jin)
Pull request description:
This change is progress towards #18771 . It adapts the fee calculation part of #16083 and addresses some feedback. The additional "verbosity level 3" features are planned for a future PR.
**Original PR description:**
> Using block undo data (like in #14802) we can now show fee information for each transaction in a block without the need for additional -txindex and/or a ton of costly lookups. For a start we'll add transaction fee information to getblock verbosity level 2. This comes at a negligible speed penalty (<1%).
ACKs for top commit:
luke-jr:
tACK 66d012ad7f
fjahr:
tACK 66d012ad7f
MarcoFalke:
review ACK 66d012ad7f 🗜
Tree-SHA512: be1fe4b866946a8dc36427f7dc72a20e10860e320a28fa49bc85bd2a93a0d699768179be29fa52e18b2ed8505d3ec272e586753ef2239b4230e0aefd233acaa2
This moves the CBlockPolicyEstimator to the NodeContext, which get rids
of two globals and allows us to conditionally create the
CBlockPolicyEstimator (and to remove a circular dep).
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
0e2a5e448f tests: dumping and minimizing of script assets data (Pieter Wuille)
4567ba034c tests: add generic qa-asset-based script verification unit test (Pieter Wuille)
f06e6d0345 tests: functional tests for Schnorr/Taproot/Tapscript (Pieter Wuille)
3c226639eb tests: add BIP340 Schnorr signature support to test framework (Pieter Wuille)
206fb180ec --- [TAPROOT] Tests --- (Pieter Wuille)
d7ff237f29 Activate Taproot/Tapscript on regtest (BIP 341, BIP 342) (Pieter Wuille)
e9a021d7e6 Make Taproot spends standard + policy limits (Pieter Wuille)
865d2c37e2 --- [TAPROOT] Regtest activation and policy --- (Pieter Wuille)
72422ce396 Implement Tapscript script validation rules (BIP 342) (Johnson Lau)
330de894a9 Use ScriptExecutionData to pass through annex hash (Pieter Wuille)
8bbed4b7ac Implement Taproot validation (BIP 341) (Pieter Wuille)
0664f5fe1f Support for Schnorr signatures and integration in SignatureCheckers (BIP 340) (Pieter Wuille)
5de246ca81 Implement Taproot signature hashing (BIP 341) (Johnson Lau)
9eb590894f Add TaggedHash function (BIP 340) (Pieter Wuille)
450d2b2371 --- [TAPROOT] BIP340/341/342 consensus rules --- (Pieter Wuille)
5d62e3a68b refactor: keep spent outputs in PrecomputedTransactionData (Pieter Wuille)
8bd2b4e784 refactor: rename scriptPubKey in VerifyWitnessProgram to exec_script (Pieter Wuille)
107b57df9f scripted-diff: put ECDSA in name of signature functions (Pieter Wuille)
f8c099e220 --- [TAPROOT] Refactors --- (Pieter Wuille)
Pull request description:
This is an implementation of the Schnorr/taproot consensus rules proposed by BIPs [340](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), [341](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki), and [342](https://github.com/bitcoin/bips/blob/master/bip-0340.mediawiki).
See the list of commits [below](https://github.com/bitcoin/bitcoin/pull/19953#issuecomment-691815830). No signing or wallet support of any kind is included, as testing is done entirely through the Python test framework.
This is a successor to https://github.com/bitcoin/bitcoin/pull/17977 (see discussion following [this comment](https://github.com/bitcoin/bitcoin/pull/17977#issuecomment-682285983)), and will have further changes squashed/rebased. The history of this PR can be found in #19997.
ACKs for top commit:
instagibbs:
reACK 0e2a5e448f
benthecarman:
reACK 0e2a5e4
kallewoof:
reACK 0e2a5e448f
jonasnick:
ACK 0e2a5e448f almost only looked at bip340/libsecp related code
jonatack:
ACK 0e2a5e448f modulo the last four commits (tests) that I plan to finish reviewing tomorrow
fjahr:
reACK 0e2a5e448f
achow101:
ACK 0e2a5e448f
Tree-SHA512: 1b00314450a2938a22bccbb4e177230cf08bd365d72055f9d526891f334b364c997e260c10bc19ca78440b6767712c9feea7faad9a1045dd51a5b96f7ca8146e
Using the zmq notifications to avoid excessive mempool polling can be difficult
given the current notifications available. It announces all transactions
being added to mempool or included in blocks, but announces no evictions
and gives no indication if the transaction is in the mempool or a block.
Block notifications for zmq are also substandard, in that it only announces
block tips, while all block transactions are still announced.
This commit adds a unified stream which can be used to closely track mempool:
1) getrawmempool to fill out mempool knowledge
2) if txhash is announced, add or remove from set
based on add/remove flag
3) if blockhash is announced, get block txn list,
remove from those transactions local view of mempool
4) if we drop a sequence number, go to (1)
The mempool sequence number starts at the value 1, and
increments each time a transaction enters the mempool,
or is evicted from the mempool for any reason, including
block inclusion. The mempool sequence number is published
via ZMQ for any transaction-related notification.
These features allow for ZMQ/RPC consumer to track mempool
state in a more exacting way, without unnecesarily polling
getrawmempool. See interface_zmq.py::test_mempool_sync for
example usage.
fa6bb0ce5d Assert that RPCArg names are equal to CRPCCommand ones (rawtransaction) (MarcoFalke)
fa80c81487 Assert that RPCArg names are equal to CRPCCommand ones (blockchain) (MarcoFalke)
Pull request description:
This is split out from #18531 to just touch some RPC methods. Description from the main pr:
### Motivation
RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.
### Changes
The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.
### Future work
> Here or follow up, makes sense to also assert type of returned UniValue?
Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:
* Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
* Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
* Auto-formatting and sanity checking the RPCExamples with RPCMan
* Checking passed-in json in self-check. Removing redundant checks
* Checking returned json against documentation to avoid regressions or false documentation
* Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static
### Bugs found
* The assert identified issue #18607
* The changes itself fixed bug #19250
ACKs for top commit:
fjahr:
utACK fa6bb0ce5d
tryphe:
utACK fa6bb0ce5d. Reducing data duplication is nice. Code changes are minimal and concise.
Tree-SHA512: deb0edc3f999baf055526eaa199b98c500635e12502dece7aa3cad5319db330eb5ee7459a5c8f040a83671a7f20c560c19a2026fb76c8416f138aa332727cbce
314b49bd50 gui: Fix regression in GUI console (Hennadii Stepanov)
Pull request description:
The regression was introduced in #19056: if the GUI is running without `-server=1`, the `*txoutset*` call in the console returns "Shutting down".
Fix #19255.
ACKs for top commit:
ryanofsky:
Code review ACK 314b49bd50. Only change since last review is rebase
Tree-SHA512: 8ff85641a5c249858fecb1ab69c7a1b2850af651ff2a94aa41ce352b5b5bc95bc45c41e1767e871b51e647612d09e4d54ede3e20c313488afef5678826c51b62
40506bf93f test: Test gettxouttsetinfo hash_type option (Fabian Jahr)
f17a4d1c4d rpc: Add hash_type NONE to gettxoutsetinfo (Fabian Jahr)
a712cf6f68 rpc: gettxoutsetinfo can specify hash_type (only legacy option for now) (Fabian Jahr)
605884ef21 refactor: Extract GetBogoSize function (Fabian Jahr)
Pull request description:
This is another intermediate part of the Coinstats Index (tracked in #18000).
Sjors suggested [here](https://github.com/bitcoin/bitcoin/pull/18000#issuecomment-641423019) that the part of the changes in #19145 that don't rely on the new `hash_type` muhash, i.e. that are for `hash_type=none`, could be merged separately from everything involving muhash. So these changes are extracted from #19145 here and can be merged without any other requirements.
Building the index with no UTXO set hash is still valuable because `gettxoutsetinfo` can still be used to audit the `total_amount` for example. By itself this PR is not a huge improvement, `hash_type=none` is speeding up `gettxoutsetinfo` by about 10%, but it enables the implementation of an index on top of it in a follow-up and that means large parts of the index code of Coinstats Index can be merged while reviews for the hashing algorithm might take longer.
ACKs for top commit:
MarcoFalke:
ACK 40506bf93f 🖨
Sjors:
tACK 40506bf93f
Tree-SHA512: 3964c2b8eed427511b1aa9b2ef285dff27dc4d1537d72c3911e435b6e6b40912232da4acb3a09bd19a0372ddffa44103388d8a650169d95a4a727b970d210add
fab80fef61 refactor: Remove unused EnsureChainman (MarcoFalke)
fa34587f1c scripted-diff: Replace EnsureChainman with Assert in unit tests (MarcoFalke)
fa6ef701ad util: Add Assert identity function (MarcoFalke)
fa457fbd33 move-only: Move NDEBUG compile time check to util/check (MarcoFalke)
Pull request description:
The utility function is primarily useful to dereference pointer types, which are known to be not null at that time.
For example, the ArgsManager is known to exist when the wallets are started: https://github.com/bitcoin/bitcoin/pull/18923/files#diff-fdb2a1a1d8bc790fcddeb6cf5a42ac55R503 . Instead of silently relying on that assumption, `Assert` can be used to abort the program and avoid UB should the assumption ever be violated.
ACKs for top commit:
promag:
Tested ACK fab80fef61.
ryanofsky:
Code review ACK fab80fef61
Tree-SHA512: 830fba10152ba17d47c4dd42809c7e26f9fe6d38e17a2d5b3f054fd644a5c4c9841286ac421ec9bb28cea9f5faeb659740fcf00de6cc589d423fee7694c42d16
9e1cb1adf1 [trivial/doc] Fix comment type (Amiti Uttarwar)
8f30260a67 [doc] Update unbroadcast description in RPC results (Amiti Uttarwar)
750456d6f2 [trivial] Remove misleading 'const' (Amiti Uttarwar)
fa32e676e5 [test] Manage node connections better in mempool persist test (Amiti Uttarwar)
1f94bb0c74 [doc] Provide rationale for randomization in scheduling. (Amiti Uttarwar)
9c8a55d9cb [mempool] Don't throw expected error message when upgrading (Amiti Uttarwar)
ba54983182 [test] Test that wallet transactions aren't rebroadcast before 12 hours (Amiti Uttarwar)
00d44a534b [test] P2P connection behavior should meet expectations (Amiti Uttarwar)
bd093ca15d [test] updates to unbroadcast test (Amiti Uttarwar)
dab298d9ab [docs] add release notes (Amiti Uttarwar)
Pull request description:
This PR is a follow up to #18038 which introduced the idea of an unbroadcast set & focuses mostly on documentation updates and test fixes. One small functionality update to not throw an expected error in `LoadMempool` when you upgrade software versions.
#18895 is another follow up to that addresses other functionality updates.
Background context:
The unbroadcast set is a mechanism for the mempool to track locally submitted transactions (via wallet or RPC). The node does a best-effort of delivering the transactions to the network via retries every 10-15 minutes until either a `GETDATA` is received or the transaction is removed from the mempool.
ACKs for top commit:
MarcoFalke:
ACK 9e1cb1adf1 👁
gzhao408:
ACK [`9e1cb1a`](9e1cb1adf1)
Tree-SHA512: 0cd51c4ca368b9dce92d50d73ec6e9df278a259e609eef2858f24cb8595ad07acc3db781d9eb0c351715f18fca5a2b4526838981fdb34a522427e9dc868bdaa6
5478d6c099 logging: thread safety annotations (Anthony Towns)
e685ca1992 util/system.cpp: add thread safety annotations for dir_locks (Anthony Towns)
a788789948 test/checkqueue_tests: thread safety annotations (Anthony Towns)
479c5846f7 rpc/blockchain.cpp: thread safety annotations for latestblock (Anthony Towns)
8b5af3d4c1 net: fMsgProcWake use LOCK instead of lock_guard (Anthony Towns)
de7c5f41ab wallet/wallet.h: Remove mutexScanning which was only protecting a single atomic bool (Anthony Towns)
c3cf2f5501 rpc/blockchain.cpp: Remove g_utxosetscan mutex that is only protecting a single atomic variable (Anthony Towns)
Pull request description:
In a few cases we need to use `std::mutex` rather than the sync.h primitives. But `std::lock_guard<std::mutex>` doesn't include the clang thread safety annotations unless you also use clang's C library, which means you can't indicate when variables should be guarded by `std::mutex` mutexes.
This adds an annotated version of `std::lock_guard<std::mutex>` to threadsafety.h to fix that, and modifies places where `std::mutex` is used to take advantage of the annotations.
It's based on top of #16112, and turns the thread safety comments included there into annotations.
It also changes the RAII classes in wallet/wallet.h and rpc/blockchain.cpp to just use the atomic<bool> flag for synchronisation rather than having a mutex that doesn't actually guard anything as well.
ACKs for top commit:
MarcoFalke:
ACK 5478d6c099🗾
hebasto:
re-ACK 5478d6c099, only renamed s/`MutexGuard`/`LockGuard`/, and dropped the commit "test/util_threadnames_tests: add thread safety annotations" since the [previous](https://github.com/bitcoin/bitcoin/pull/16127#pullrequestreview-414184113) review.
ryanofsky:
Code review ACK 5478d6c099. Thanks for taking suggestions! Only changes since last review are dropping thread rename test commit d53072ec730d8eec5a5b72f7e65a54b141e62b19 and renaming mutex guard to lock guard
Tree-SHA512: 7b00d31f6f2b5a222ec69431eb810a74abf0542db3a65d1bbad54e354c40df2857ec89c00b4a5e466c81ba223267ca95f3f98d5fbc1a1d052a2c3a7d2209790a
fa756928c3 rpc: Make gettxoutsetinfo/GetUTXOStats interruptible (MarcoFalke)
fa7fc5a8e0 rpc: factor out RpcInterruptionPoint from dumptxoutset (MarcoFalke)
Pull request description:
Make it interruptible, so that shutdown doesn't block for up to one hour.
Fixes (partially) #13217
ACKs for top commit:
Empact:
Code Review ACK fa756928c3
laanwj:
Code review ACK fa756928c3
Tree-SHA512: 298261e0ff7d79fab542b8f6828cc0ac451cbafe396d5f0816c9d36437faba1330f5c4cb2a25c5540e202bfb9783da6ec858bd453056ce488d21e36335d3d42c
fab6b9d18f validation: Mark g_chainman DEPRECATED (MarcoFalke)
fa1d97b256 validation: Make ProcessNewBlock*() members of ChainstateManager (MarcoFalke)
fa24d49098 validation: Make PruneOneBlockFile() a member of ChainstateManager (MarcoFalke)
fa84b1cd84 validation: Make LoadBlockIndex() a member of ChainstateManager (MarcoFalke)
fa05fdf0f1 net: Pass chainman into PeerLogicValidation (MarcoFalke)
fa7b626d7a node: Add chainman alias for g_chainman (MarcoFalke)
Pull request description:
The global `g_chainman` has recently been introduced in #17737. The chainstate manager is primarily needed for the assumeutxo feature, but it can also simplify testing in the future.
The goal of this pull is to make the global chainstate manager internal to validation, so that all external code does not depend on globals and that unit or fuzz tests can pass in their (potentially mocked) chainstate manager.
I suggest reviewing the pull request commit-by-commit. It should be relatively straightforward refactoring that does not change behavior at all.
ACKs for top commit:
ryanofsky:
Code review ACK fab6b9d18f. Had to be rebased but still looks good
Tree-SHA512: dcbf114aeef4f8320d466369769f22ce4dd8f46a846870354df176c3de9ff17c64630fbd777e7121d7470d7a8564ed8d37b77168746e8df7489c6877e55d7b4f
651f1d816f [test] wait for inital broadcast before comparing mempool entries (gzhao408)
9d3f7eb986 [mempool] sanity check that all unbroadcast txns are in mempool (gzhao408)
a7ebe48b94 [rpc] add unbroadcast info to mempool entries and getmempoolinfo (gzhao408)
d160069604 [wallet] remove nLastResend logic (gzhao408)
Pull request description:
Followup to #18038 by amitiuttarwar which introduces the unbroadcast set: "a mechanism for the mempool to track locally submitted transactions" and decreases the frequency of rebroadcast from 10-15 minutes to 12-36 hours.
This PR addresses some of the outstanding TODOs building on top of it:
- remove `nLastResend` logic, which is used to ensure rebroadcast doesn't happen again if no new block has been mined (makes sense in 10-15 min period, but not necessary for 12-36 hour period). (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416826914))
- expose unbroadcast info via RPCs, for more informative queries and testing (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416837980))
- add sanity check to verify unbroadcast transactions still exist in mempool before rebroadcasting (#18038 [comment](https://github.com/bitcoin/bitcoin/pull/18038#discussion_r416861609))
ACKs for top commit:
naumenkogs:
Code review ACK 651f1d816f
amitiuttarwar:
ACK 651f1d816f🎉
MarcoFalke:
Review ACK 651f1d816f
Tree-SHA512: d5327e95ef39d44152b48df5c610502ae11c168f43dbbfb2885340c93d1ba9426eb3a5794573f5fc843502109cb3ffb63efa3f2db4f8f112efcde8f76d9a8845
b3f7f375ef refactor: Remove g_rpc_node global (Russell Yanofsky)
ccb5059ee8 scripted-diff: Remove g_rpc_node references (Russell Yanofsky)
6fca33b2ed refactor: Pass NodeContext to RPC and REST methods through util::Ref (Russell Yanofsky)
691c817b34 Add util::Ref class as temporary alternative for c++17 std::any (Russell Yanofsky)
Pull request description:
This PR removes the `g_rpc_node` global, to get same benefits we see removing other globals and make RPC code more testable, modular, and reusable.
This uses a hybrid of the approaches suggested in #17548. Instead of using `std::any`, which isn't available in c++11, or `void*`, which isn't type safe, it uses a small new `util::Ref` helper class, which acts like a simplified `std::any` that only holds references, not values.
Motivation for writing this was to provide an simpler alternative to #18647 by Harris Brakmić (brakmic) which avoids some shortcomings of that PR (https://github.com/bitcoin/bitcoin/pull/18647#issuecomment-617878826)
ACKs for top commit:
MarcoFalke:
re-ACK b3f7f375ef, only change is adding back const and more tests 🚾
ajtowns:
ACK b3f7f375ef
Tree-SHA512: 56292268a001bdbe34d641db1180c215351503966ff451e55cc96c9137f1d262225d7d7733de9c9da7ce7d7a4b34213a98c2476266b58c89dbbb0f3cb5aa5d70
- expose info about number of txns in unbroadcast set and whether a mempool entry's tx has passed initial broadcast
- makes rpcs more informative and allows for more explicit testing, eg tracking if tx is in unbroadcast set
before and after originating node connects to peers (adds this in mempool_unbroadcast.py)
- adds mempool method IsUnbroadcastTx to query for tx inclusion in mempool's unbroadcast set
