This creates a cleaner interface with ATMP, allows us to make results const,
and makes accessing values that don't make sense (e.g. fee when tx is
invalid) an error.
Since these chainstates are:
1. Also vulnerable to the race condition described in the previous
commit
2. Documented as having similar semantics as m_active_chainstate
we should also protect them with ::cs_main.
67c9a83df1 style-only: Remove redundant sentence in ActivateBestChain comment (Carl Dong)
b8e95658d5 style-only: Make TestBlockValidity signature readable (Carl Dong)
0cdad75390 validation: Use accessible chainstate in ChainstateManager::ProcessNewBlock (Carl Dong)
ea4fed9021 validation: Use existing chainstate in ChainstateManager::ProcessNewBlockHeaders (Carl Dong)
e0dc305727 validation: Move LoadExternalBlockFile to CChainState (Carl Dong)
5f8cd7b3a5 validation: Remove global ::ActivateBestChain (Carl Dong)
2a696472a1 validation: Pass in chainstate to ::NotifyHeaderTip (Carl Dong)
9c300cc8b3 validation: Pass in chainstate to TestBlockValidity (Carl Dong)
0e17c833cd validation: Make CChainState.m_blockman public (Carl Dong)
d363d06bf7 validation: Pass in blockman to ContextualCheckBlockHeader (Carl Dong)
f11d11600d validation: Move GetLastCheckpoint to BlockManager (Carl Dong)
e4b95eefbc validation: Move GetSpendHeight to BlockManager (Carl Dong)
b026e318c3 validation: Move FindForkInGlobalIndex to BlockManager (Carl Dong)
3664a150ac validation: Remove global LookupBlockIndex (Carl Dong)
eae54e6e60 scripted-diff: Use BlockManager::LookupBlockIndex (Carl Dong)
15d20f40e1 validation: Move LookupBlockIndex to BlockManager (Carl Dong)
f92dc6557a validation: Guard the active_chainstate with cs_main (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
jnewbery:
utACK 67c9a83df1
laanwj:
re-ACK 67c9a83df1
ryanofsky:
Code review ACK 67c9a83df1. Changes since last review:
Tree-SHA512: 8744aba2dd57a40cd2fedca809b0fe24d771bc60da1bffde89601999384aa0df428057a86644a3f72fbeedbc8b04db6c4fd264ea0db2e73c279e5acc6d056cbf
c943282b5e validation: remove redundant check on pindex (jarolrod)
Pull request description:
This removes a redundant check on `pindex` being a `nullptr`. By the time we get to this step `pindex` is always a `nullptr` as the branch where it has been set would have already returned.
Closes#19223
ACKs for top commit:
Zero-1729:
re-ACK c943282
ajtowns:
ACK c943282b5e - code review only
MarcoFalke:
review ACK c943282b5e📨
theStack:
re-ACK c943282b5e
Tree-SHA512: d2dc58206be61d2897b0703ee93af67abed492a80e84ea03dcfbf7116833173da3bdafa18ff80422d5296729d5254d57cc1db03fdaf817c8f57c62c3abef673c
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
LoadExternalBlockFile mainly acts on CChainState.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
GetLastCheckPoint mainly acts on BlockManager.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
GetSpendHeight only acts on BlockManager.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
FindForkInGlobalIndex only acts on BlockManager.
Note to reviewers: Since FindForkInGlobalIndex is always called with
::ChainActive() as its first parameter, it is possible to move
FindForkInGlobalIndex to CChainState and remove this const CChain&
parameter to instead use m_chain. However, it seems like the original
intention was for FindForkInGlobalIndex to work with _any_ chain, not
just the current active chain. Let me know if this should be changed.
[META] This commit should be followed up by a scripted-diff commit which
fixes calls to LookupBlockIndex tree-wide.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
LookupBlockIndex only acts on BlockManager.
bb6fcc75d1 refactor: Drop boost::thread stuff in CCheckQueue (Hennadii Stepanov)
6784ac471b bench: Use CCheckQueue local thread pool (Hennadii Stepanov)
dba30695fc test: Use CCheckQueue local thread pool (Hennadii Stepanov)
01511776ac Add local thread pool to CCheckQueue (Hennadii Stepanov)
0ef938685b refactor: Use member initializers in CCheckQueue (Hennadii Stepanov)
Pull request description:
This PR:
- gets rid of `boost::thread_group` in the `CCheckQueue` class
- allows thread safety annotation usage in the `CCheckQueue` class
- is alternative to #14464 (https://github.com/bitcoin/bitcoin/pull/18710#issuecomment-616618525, https://github.com/bitcoin/bitcoin/pull/18710#issuecomment-617291612)
Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938.
Related: #17307
ACKs for top commit:
laanwj:
Code review ACK bb6fcc75d1
LarryRuane:
ACK bb6fcc75d1
jonatack:
Code review ACK bb6fcc75d1 and verified rebase to master builds cleanly with unit/functional tests green
Tree-SHA512: fddeb720d5a391b48bb4c6fa58ed34ccc3f57862fdb8e641745c021841c8340e35c5126338271446cbd98f40bd5484f27926aa6c3e76fa478ba1efafe72e73c1
This removes a conditional that checks if pindex is equal to nullptr.
This check is redundant because the branch where pindex is set returns at an earlier time. Additionaly, The independence of the earlier and later pindex is made clearer.
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
In 0.21, we added unbroadcast txids to mempool.dat. Commit 9c8a55d
added a try-block to prevent throwing a "failed to deserialize mempool data"
error when a user upgrades from 0.21 to 0.22. This exception handling is no
longer useful, so now we can remove it.
4e28753f60 feestimator: encapsulate estimation file logic (Antoine Poinsot)
e8ea6ad9c1 init: don't create a CBlockPolicyEstimator if we don't relay transactions (Antoine Poinsot)
86ff2cf202 Remove the remaining fee estimation globals (Antoine Poinsot)
03bfeee957 interface: remove unused estimateSmartFee method from node (Antoine Poinsot)
Pull request description:
If the `blocksonly` mode is turned on after running with transaction
relay enabled for a while, the fee estimation will serve outdated data
to both the internal wallet and to external applications that might be
feerate-sensitive and make use of `estimatesmartfee` (for example a
Lightning Network node).
This has already caused issues (for example https://github.com/bitcoin/bitcoin/issues/16840 (C-lightning), or https://github.com/lightningnetwork/lnd/issues/2562 (LND)) and it seems prudent to fail rather than to give inaccurate values.
This fixes#16840, and closes#16890 which tried to fix the symptoms (RPC) but not the cause as mentioned by sdaftuar :
> If this is a substantial problem, then I would think we should take action to protect our own wallet users as well (rather than hide the results of what our fee estimation would do!).
ACKs for top commit:
MarcoFalke:
re-ACK 4e28753f60👋
jnewbery:
utACK 4e28753f60
Tree-SHA512: c869cf03b86d8194002970bbc84662dae76874967949b9be0d9a4511a1eabcb1627c38aca3154da9dcece1a4c49ec02bd4f9fcca2ec310986e07904559e63ba8
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>
0000a0c7e9 Remove confusing and almost useless "unexpected version" warning (MarcoFalke)
Pull request description:
It is useless because it isn't displayed for most users:
* It isn't displayed in normal operation (because the validation debug category is disabled by default)
* It isn't displayed for users that sync up their nodes intermittently, e.g. once a day or once a week (because it is disabled for IBD)
* It is only displayed in the debug log (as opposed to the versionbits warning, which is displayed more prominently)
It is confusing because it doesn't have a use case:
Despite the above, if a user *did* see the warning, it would most likely be a false positive (like it has been in the past). Even if it wasn't, there is nothing they can do about it. The only thing they could do is to check for updates and hope that a fixed version is available. But why would the user be so scrupulously precise in enabling the warning and reading the log, but then fail to regularly check update channels for updated software?
ACKs for top commit:
practicalswift:
ACK 0000a0c7e9
decryp2kanon:
ACK 0000a0c
LarryRuane:
ACK 0000a0c7e9
Tree-SHA512: 16e069c84be6ab6034baeefdc515d0e5cdf560b2005d2faec5f989d45494bd16cfcb4ffca6a17211d9556ae44f9737a60a476c08b5c2bb5e1bd29724ecd6d5c1
fa7eed5be7 doc: Clarify that vpindexToConnect is in reverse order (MarcoFalke)
fa62304c97 Remove dead CheckForkWarningConditionsOnNewFork (MarcoFalke)
Pull request description:
The function has several code and logic bugs, which prevent it from working at all:
* `vpindexToConnect.back()` is passed to `CheckForkWarningConditionsOnNewFork`, which is the earliest connected block (least work block), *not* the new fork tip
* `ActivateBestChainStep` will never try to connect a block that descends from an invalid block, so the invalid fork will only ever be of height 1, never hitting the 7 block minimum condition
Instead of dragging the dead and wrong code around through every change in validation, remove it. In the future it could make sense to add a fork detection somewhere outside of the `ActivateBestChainStep` logic (maybe net_processing).
ACKs for top commit:
jnewbery:
utACK fa7eed5be7
fjahr:
Code review ACK fa7eed5be7
glozow:
utACK fa7eed5be7 I see that it's dead code
Tree-SHA512: 815bdbac7c1eb5b7594b0866a2dbd3c7619797afaadb03a5269fb96739ffb83b05b8e4f7c1e68d48d7886132dd0b12c14c3fb4ee0e72de1074726050ed203e1a
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
This implements the new sighashing scheme from BIP341, with all relevant
whole-transaction values precomputed once and cached.
Includes changes to PrecomputedTransactionData by Pieter Wuille.
A BIP-341 signature message may commit to the scriptPubKeys and amounts
of all spent outputs (including other ones than the input being signed
for spends), so keep them available to signature hashing code.
Check absurd fee in BroadcastTransaction and RPC,
return TransactionError::MAX_FEE_EXCEEDED instead
of TxValidationResult::TX_NOT_STANDARD because this
is client preference, not a node-wide policy.
62dba9628d log: print unexpected version warning in validation log category (nthumann)
Pull request description:
Fixes#19603: As suggested by practicalswift, instead of always printing `<n> of the last 100 blocks have unexpected version` as a warning appended to UpdateTip, it is now printed in the validation log category and therefore only visible with `-debug=validation` enabled.
Before:
`2020-09-06T15:56:00Z UpdateTip: new best=00000000000000000001b2872e107a98b57913120e5c6c87ce2715a34c40adf8 height=646969 version=0x20400000 log2_work=92.261571 tx=565651941 date='2020-09-06T10:35:36Z' progress=0.999888 cache=32.2MiB(237417txo) warning='72 of last 100 blocks have unexpected version'`
After:
`2020-09-06T16:31:26Z UpdateTip: new best=0000000000000000000b3bd786dc42745dd7be4a8c695500a04518cb9e2f4dc1 height=646971 version=0x20000000 log2_work=92.261607 tx=565655901 date='2020-09-06T10:57:19Z' progress=0.999883 cache=3.8MiB(27550txo)`
`2020-09-06T16:31:26Z 71 of last 100 blocks have unexpected version`
Ran unit & functional tests, confirmed that the warning is now only printed when validation category is enabled.
ACKs for top commit:
theStack:
ACK 62dba9628d
MarcoFalke:
re-ACK 62dba96
practicalswift:
ACK 62dba9628d -- only change since last ACK is `s/nUpgraded/num_unexpected_version/`
hebasto:
re-ACK 62dba9628d, https://github.com/bitcoin/bitcoin/pull/19898#pullrequestreview-483158708 is resolved now.
Tree-SHA512: 2100ca7d6d3fd67c92e81d75162d2506d6f1ecf1761d5180d76663fac06771b35e5c4235ebe1a00731b5f7db82db3cd19328627929c8f22912df592686ba51d3
72a1d5c6f3 validation: Remove review-only comments + assertions (Carl Dong)
3756853b15 docs: Move FindFilesToPrune{,Manual} doxygen comment (Carl Dong)
485899a93c style: Make FindFilesToPrune{,Manual} match style guide (Carl Dong)
3f5b5f3f6d validation: Move FindFilesToPrune{,Manual} to BlockManager (Carl Dong)
f8d4975ab3 validation: Move PruneOneBlockFile to BlockManager (Carl Dong)
74f73c783d validation: Pass in chainman to UnloadBlockIndex (Carl Dong)
4668ded6d6 validation: Move ~CMainCleanup logic to ~BlockManager (Carl Dong)
Pull request description:
This PR paves the way for de-globalizing `g_chainman` entirely by removing the usage of `g_chainman` in the following functions/methods:
- `~CMainCleanup`
- `CChainState::FlushStateToDisk`
- `UnloadBlockIndex`
The remaining direct uses of `g_chainman` are as follows:
1. In initialization codepaths:
- `AppTests`
- `AppInitMain`
- `TestingSetup::TestingSetup`
2. `::ChainstateActive`
3. `LookupBlockIndex`
- Note: `LookupBlockIndex` is used extensively throughout the codebase and require a much larger set of changes, therefore I've left it out of this initial PR
ACKs for top commit:
MarcoFalke:
re-ACK 72a1d5c6f3👚
jnewbery:
utACK 72a1d5c6f3
Tree-SHA512: 944a4fa8405eecf39706ff944375d6824373aaeea849d11473f08181eff26b12f70043a8348a5b08e6e9021b243b481842fbdfbc7c3140ca795fce3688b7f5c3
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.
[META] This is a followup to "validation: Move FindFilesToPrune{,Manual}
to BlockManager" removing comments and assertions meant only to
show that the change is correct.
[META] No behaviour change is intended in this commit.
[META] This commit should be followed up by removing the comments and
assertions meant only to show that the change is correct.
Also stop FindFilesToPrune{,Manual} from unnecessary reaching for
::ChainActive() by passing in the necessary information.
23c35bf005 [test] add get_vsize util for more programmatic testing (gzhao408)
2233a93a10 [rpc] Return fee and vsize from testmempoolaccept (codeShark149)
Pull request description:
From #19093 and resolves#19057.
Difference from #19093: return `vsize` and `fees` object (similar to `getmempoolentry`) when the test accept is successful. Updates release-notes.md.
ACKs for top commit:
jnewbery:
utACK 23c35bf005
fjahr:
utACK 23c35bf
instagibbs:
reACK 23c35bf005
Tree-SHA512: dcb81b7b817a4684e9076bc5d427a6f2d549d2edc66544e718260c4b5f8f1d5ae1d47b754175e9f0c8a3bd8371ce116c2dca0583588d513a7d733d5d614f2b04
a8a64acaf3 [BroadcastTransaction] Remove unsafe move operator (Amiti Uttarwar)
125c038126 [p2p] Remove dead code (Amiti Uttarwar)
fc66d0a65c [p2p] Check for nullptr before dereferencing pointer (Adam Jonas)
cb79b9dbf4 [mempool] Revert unbroadcast set to tracking just txid (Amiti Uttarwar)
Pull request description:
Addresses some outstanding review comments from #18044
- reverts unbroadcast txids to a set instead of a map (simpler, communicates intent better, takes less space, no efficiency advantages of map)
- adds safety around two touchpoints (check for nullptr before dereferencing pointer, remove an inaccurate std::move operator)
- removes some dead code
Links to comments on wtxid PR: [1](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460495254) [2](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460496023) [3](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r463532611)
thanks to jnewbery & adamjonas for flagging these ! !
ACKs for top commit:
sdaftuar:
utACK a8a64acaf3
naumenkogs:
utACK a8a64acaf3
jnewbery:
utACK a8a64acaf3
Tree-SHA512: 7be669cb30cc17fb9e06b50e636ef7887c6a27354697987e4e4d38dba4b8f50e175647587430cd9bc3295bec01ce8b1e6639a50a4249d8fff9b1ca1b9ead3277
[META] This is a pure refactor commit.
Move PruneBlockFile to BlockManager because:
1. PruneOneBlockFile only acts on BlockManager
2. Eliminates the need for callers (FindFilesToPrune{,Manual}) to have a
reference to the larger ChainstateManager, just a reference to
BlockManager is enough. See following commits.
~CMainCleanup:
1. Is vestigial
2. References the g_chainman global (we should minimize g_chainman refs)
3. Only acts on g_chainman.m_blockman
4. Does the same thing as BlockManager::Unload
Instead of printing "<n> of the last 100 blocks have unexpected version"
as a warning appended to UpdateTip, it is now printed in the validation
log category.
When I originally implemented the unbroadcast set in 18038, it just tracked
txids. After 18038 was merged, I offered a patch to 18044 to make the
unbroadcast changes compatible with wtxid relay. In this patch, I updated
`unbroadcast_txids` to a map of txid -> wtxid. Post merge review comments shed
light on the fact that this update was unnecessary, and distracting. So, this
commit updates the unbroadcast ids back to a set.
0a8aa626dd refactor: Make HexStr take a span (Wladimir J. van der Laan)
Pull request description:
Make `HexSt`r take a span of bytes, instead of an awkward pair of templated iterators. This simplifies most of the uses.
ACKs for top commit:
elichai:
Code review ACK 0a8aa626dd
hebasto:
re-ACK 0a8aa626dd
jonatack:
re-ACK 0a8aa626dd
Tree-SHA512: 6e178ece5cbac62119c857a10299b1e85422938084c3f03063e17119a5129e0c28016e05a6fabaa4c271a7e0a37c7cd89fa47c435ee19b38a5acfe80d00de992
9f88ded82b test addition of unknown segwit spends to txid reject filter (Gregory Sanders)
7989901c7e Add txids with non-standard inputs to reject filter (Suhas Daftuar)
Pull request description:
Our policy checks for non-standard inputs depend only on the non-witness
portion of a transaction: we look up the scriptPubKey of the input being
spent from our UTXO set (which is covered by the input txid), and the p2sh
checks only rely on the scriptSig portion of the input.
Consequently it's safe to add txids of transactions that fail these checks to
the reject filter, as the witness is irrelevant to the failure. This is helpful
for any situation where we might request the transaction again via txid (either
from txid-relay peers, or if we might fetch the transaction via txid due to
parent-fetching of orphans).
Further, in preparation for future witness versions being deployed on the
network, ensure that WITNESS_UNKNOWN transactions are rejected in
AreInputsStandard(), so that transactions spending v1 (or greater) witness
outputs will fall into this category of having their txid added to the reject
filter.
ACKs for top commit:
ajtowns:
ACK 9f88ded82b - code review
jnewbery:
Code review ACK 9f88ded82b
ariard:
Code Review/Tested ACK 9f88ded
naumenkogs:
utACK 9f88ded82b
jonatack:
ACK 9f88ded82b
Tree-SHA512: 1e93c0a5b68cb432524780ffc0093db893911fdfed9e2ed17f888e59114cc75d2a07062aefad4e5ce2e87c9270886117a8abb3c78fb889c9b9f31967f1777148
Our policy checks for non-standard inputs depend only on the non-witness
portion of a transaction: we look up the scriptPubKey of the input being
spent from our UTXO set (which is covered by the input txid), and the p2sh
checks only rely on the scriptSig portion of the input.
Consequently it's safe to add txids of transactions that fail these checks to
the reject filter, as the witness is irrelevant to the failure. This is helpful
for any situation where we might request the transaction again via txid (either
from txid-relay peers, or if we might fetch the transaction via txid due to
parent-fetching of orphans).
Further, in preparation for future witness versions being deployed on the
network, ensure that WITNESS_UNKNOWN transactions are rejected in
AreInputsStandard(), so that transactions spending v1 (or greater) witness
outputs will fall into this category of having their txid added to the reject
filter.
f19fdd47a6 test: add test for CChainState::ResizeCoinsCaches() (James O'Beirne)
8ac3ef4699 add ChainstateManager::MaybeRebalanceCaches() (James O'Beirne)
f36aaa6392 Add CChainState::ResizeCoinsCaches (James O'Beirne)
b223111da2 txdb: add CCoinsViewDB::ChangeCacheSize (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
In the assumeutxo implementation draft (#15056), once a UTXO snapshot is loaded, a new chainstate object is created after initialization. This means that we have to reclaim some of the cache that we've allocated to the original chainstate (per `dbcache=`) to repurpose for the snapshot chainstate.
Furthermore, it makes sense to have different cache allocations depending on which chainstate is more active. While the snapshot chainstate is working to get to the network tip (and the background validation chainstate is idle), it makes sense that the snapshot chainstate should have the majority of cache allocation. And contrariwise once the snapshot has reached network tip, most of the cache should be given to the background validation chainstate.
This set of changes (detailed in the commit messages) allows us to dynamically resize the various coins caches. None of the functionality introduced here is used at the moment, but will be in the next AU PR (which introduces `ActivateSnapshot`).
`ChainstateManager::MaybeRebalanceCaches()` defines the (somewhat normative) cache allocations between the snapshot and background validation chainstates. I'd be interested in feedback if anyone has thoughts on the proportions I've set there.
ACKs for top commit:
ajtowns:
weak utACK f19fdd47a6 -- didn't find any major problems, but not super confident that I didn't miss anything
fjahr:
Code review ACK f19fdd4
ryanofsky:
Code review ACK f19fdd47a6. Only change since last review is constructor cleanup (no change in behavior). I think the suggestions here from ajtowns and others are good, but shouldn't delay merging the PR (and hold up assumeutxo)
Tree-SHA512: fffb7847fb6993dd4a1a41cf11179b211b0b20b7eb5f7cf6266442136bfe9d43b830bbefcafd475bfd4af273f5573500594aa41fff03e0ed5c2a1e8562ff9269
0a4f1422cd Further improve comments around recentRejects (Suhas Daftuar)
0e20cfedb7 Disconnect peers sending wtxidrelay message after VERACK (Suhas Daftuar)
cacd85209e test: Use wtxid relay generally in functional tests (Fabian Jahr)
8d8099e97a test: Add tests for wtxid tx relay in segwit test (Fabian Jahr)
9a5392fdf6 test: Update test framework p2p protocol version to 70016 (Fabian Jahr)
dd78d1d641 Rename AddInventoryKnown() to AddKnownTx() (Suhas Daftuar)
4eb515574e Make TX_WITNESS_STRIPPED its own rejection reason (Suhas Daftuar)
97141ca442 Delay getdata requests from peers using txid-based relay (Suhas Daftuar)
46d78d47de Add p2p message "wtxidrelay" (Suhas Daftuar)
2d282e0cba ignore non-wtxidrelay compliant invs (Anthony Towns)
ac88e2eb61 Add support for tx-relay via wtxid (Suhas Daftuar)
8e68fc246d Add wtxids to recentRejects instead of txids (Suhas Daftuar)
144c385820 Add wtxids of confirmed transactions to bloom filter (Suhas Daftuar)
85c78d54af Add wtxid-index to orphan map (Suhas Daftuar)
08b39955ec Add a wtxid-index to mapRelay (Suhas Daftuar)
60f0acda71 Just pass a hash to AddInventoryKnown (Suhas Daftuar)
c7eb6b4f1f Add wtxid to mempool unbroadcast tracking (Amiti Uttarwar)
2b4b90aa8f Add a wtxid-index to the mempool (Suhas Daftuar)
Pull request description:
Using txids (a transaction's hash, without witness) for transaction relay is problematic, post-segwit -- if a peer gives us a segwit transaction that fails policy checks, it could be because the txid associated with the transaction is definitely unacceptable to our node (regardless of the witness), or it could be that the transaction was malleated and with a different witness, the txid could be accepted to our mempool.
We have a bloom filter of recently rejected transactions, whose purpose is to help us avoid redownloading and revalidating transactions that fail to be accepted, but because of this potential for witness malleability to interfere with relay of valid transactions, we do not use the filter for segwit transactions. This issue is discussed at some length in #8279. The effect of this is that whenever a segwit transaction that fails policy checks is relayed, a node would download that transaction from every peer announcing it, because it has no way presently to cache failure. Historically this hasn't been a big problem, but if/when policy for accepting segwit transactions were to change (eg taproot, or any other change), we could expect older nodes talking to newer nodes to be wasting bandwidth because of this.
As discussed in that issue, switching to wtxid-based relay solves this problem -- by using an identifier for a transaction that commits to all the data in our relay protocol, we can be certain if a transaction that a peer is announcing is one that we've already tried to process, or if it's something new. This PR introduces support for wtxid-based relay with peers that support it (and remains backwards compatible with peers that use txids for relay, of course).
Apart from code correctness, one issue to be aware of is that by downloading from old and new peers alike, we should expect there to be some bandwidth wasted, because sometimes we might download the same transaction via txid-relay as well as wtxid-relay. The last commit in this PR implements a heuristic I want to analyze, which is to just delay relay from txid-relay peers by 2 seconds, if we have at least 1 wtxid-based peer. I've just started running a couple nodes with this heuristic so I can measure how well it works, but I'm open to other ideas for minimizing that issue. In the long run, I think this will be essentially a non-issue, so I don't think it's too big a concern, we just need to bite the bullet and deal with it during upgrade.
Finally, this proposal would need a simple BIP describing the changes, which I haven't yet drafted. However, review and testing of this code in the interim would be welcome.
To do items:
- [x] Write BIP explaining the spec here (1 new p2p message for negotiating wtxid-based relay, along with a new INV type)
- [ ] Measure and evaluate a heuristic for minimizing how often a node downloads the same transaction twice, when connected to old and new nodes.
ACKs for top commit:
naumenkogs:
utACK 0a4f1422cd
laanwj:
utACK 0a4f1422cd
Tree-SHA512: d8eb8f0688cf0cbe9507bf738e143edab1f595551fdfeddc2b6734686ea26e7f156b6bfde38bad8bbbe8bec1857c7223e1687f8f018de7463dde8ecaa8f450df
fa56eda58e log: Avoid treating remote misbehvior as local system error (MarcoFalke)
fa492895b5 refactor: Switch ValidationState mode to C++11 enum class (MarcoFalke)
Pull request description:
When logging failures of `CheckBlockHeader` (high-hash), they are always logged as system error. This is problematic for several reasons:
* Submitting a blockheader that fails `CheckBlockHeader` over RPC will result in a debug log line that starts with `ERROR`. Proper behaviour should be to log not anything and instead only return the failure reason to the RPC user. This pull does not fix this issue entirely, but is a good first step in the right direction.
* A misbehaving peer that sends us an invalid block header that fails `CheckBlockHeader` will result in a debug log line that starts with `ERROR`. Proper behavior should be to log the remote peer misbehavior if logging for that category was enabled. This pull fixes this issue for `CheckBlockHeader` and other functions can be adjusted as well if needed in follow-ups. This should be a good first step in the right direction.
ACKs for top commit:
practicalswift:
re-ACK fa56eda58e
Tree-SHA512: 9793191f5cb57bdff7c93926e94877e8ca2ef89dcebcf9eb155899c733961839ec7c3f9b9f001dc082ada4234fe6e75f6df431301678d6822325840771166d77
5fa067a27d Remove unnecessary blockfile SetPos (Tom Harding)
Pull request description:
Nothing could have changed the position since we retrieved it a few statements earlier. This dates from commit 16d5194165.
ACKs for top commit:
LarryRuane:
ACK 5fa067a27d
Tree-SHA512: 459cc7226e186c231ffb67f0613f550e8eb940f1b8933c3bc4a4e8dd519c8d5d45884e8cfd9347039dab90a093644bbbb31be063baed1c6fc7984b6cb4f17c9f
Previously, TX_WITNESS_MUTATED could be returned during transaction validation
for either transactions that had a witness that was non-standard, or for
transactions that had no witness but were invalid due to segwit validation
rules.
However, for txid/wtxid-relay considerations, net_processing distinguishes the
witness stripped case separately, because it affects whether a wtxid should be
able to be added to the reject filter. It is safe to add the wtxid of a
witness-mutated transaction to the filter (as that wtxid shouldn't collide with
the txid, and hence it wouldn't interfere with transaction relay from
txid-relay peers), but it is not safe to add the wtxid (== txid) of a
witness-stripped transaction to the filter, because that would interfere with
relay of another transaction with the same txid (but different wtxid) when
relaying from txid-relay peers.
Also updates the comment explaining this logic, and explaining that we can get
rid of this complexity once there's a sufficient deployment of wtxid-relaying
peers on the network.
fa9f20b647 log: Properly log txs rejected from mempool (MarcoFalke)
Pull request description:
Currently `CheckTxInputs` rejections from the mempool are the only rejections that log directly and unconditionally to debug.log instead of leaving it to the caller. This has multiple issues:
* A rejected RPC transaction will log a redundant failure reason to debug log. All other failures are merely reported to the RPC user.
* A rejected p2p transaction will log the failure twice. Once with the `MEMPOOLREJ` flag, and once unconditionally.
* A rejected orphan transaction will log no failure.
Fix all issues by simply returning the state to the caller, like it is done for all other rejections.
The patch includes whitespace fixups to highlight relevant parts of the codebase and simplify review.
ACKs for top commit:
naumenkogs:
utACK fa9f20b647
rajarshimaitra:
Concept ACK. Compiled and ran tests. `fa9f20b`
jnewbery:
code review ACK fa9f20b647
Tree-SHA512: 86cc17b2a9239c01c4fc3f254ad48ee1d3883266966b9811030176338b9ac3deaea7ea5babfb8bbf739d7440154e30011fede8f9313175f199d4a062af6494f7
c858302280 Change format of log2_work for uniform output (zero-padded) (jmorgan)
Pull request description:
Motivation:
It's jarring to watch the output of `tail -f ~/btcdata/debug.log` scroll by and very frequently see columns not lining up correctly because `log2_work` somtimes has less precision than 8 digits.
Current display:
```
2020-06-18T02:54:42Z UpdateTip: new best=0000000000000000107f877e4920643f9fb06090fa7551cd1cdd83b857f520aa height=382038 version=0x00000003 log2_work=83.558653 tx=90953616 date='2015-11-04T17:11:44Z' progress=0.166675 cache=117.6MiB(966410txo)
2020-06-18T02:54:51Z UpdateTip: new best=0000000000000000019a4de585d30d1a8cc13c7a1972d11b4945635c9556acb5 height=382039 version=0x00000003 log2_work=83.55868 tx=90955936 date='2015-11-04T17:19:39Z' progress=0.166679 cache=117.9MiB(968799txo)
```
Display with this commit:
```
2020-06-18T02:54:42Z UpdateTip: new best=0000000000000000107f877e4920643f9fb06090fa7551cd1cdd83b857f520aa height=382038 version=0x00000003 log2_work=83.558653 tx=90953616 date='2015-11-04T17:11:44Z' progress=0.166675 cache=117.6MiB(966410txo)
2020-06-18T02:54:51Z UpdateTip: new best=0000000000000000019a4de585d30d1a8cc13c7a1972d11b4945635c9556acb5 height=382039 version=0x00000003 log2_work=83.55868 tx=90955936 date='2015-11-04T17:19:39Z' progress=0.166679 cache=117.9MiB(968799txo)
```
ACKs for top commit:
practicalswift:
ACK c858302280 -- patch looks great :)
achow101:
ACK c858302280
laanwj:
Tested ACK c858302280
Tree-SHA512: 16cbe419c4993ad51019c676e8ca409ef1025b803cc598437c780dd7ca003d7e4ad421f451e9a374e0070ee9b3ee601b7aba849e1f346798f9321d1bce5c4401
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
fa0dfdf447 refactor: Remove confusing BlockIndex global (MarcoFalke)
Pull request description:
The global `::BlockIndex()` is problematic for several reasons:
* It returns a mutable reference to the block tree, without the appropriate lock annotation (`m_block_index` is guarded by `cs_main`). The current code is fine, but in the future this might lead to accidental races and data corruption.
* The rpc server shouldn't rely on node globals, but rather a context that is passed in to the RPC method.
* Tests might want to spin up their own block tree, and thus should also not rely on a single global.
Fix all issues by removing the global
ACKs for top commit:
promag:
Code review ACK fa0dfdf447.
jonatack:
re-ACK fa0dfdf
Tree-SHA512: 8f158fc5e1c67e73588a21c25677b3fa0fe442313b13ec24b87054806c59607d6ba0c062a865ce3e0ee568706bd0d1faa84febda21aff5bcd65dab172f74c52f
Also adds CCoinsViewCache::ReallocateCache() to attempt to free
memory that the cacheCoins's allocator may be hanging onto when
downsizing the cache.
Adds `CChainState::m_coins{tip,db}_cache_size_bytes` data members
so that we can reference cache size on a per-chainstate basis for
flushing.
faca73000f ci: Install fixed version of clang-format for linters (MarcoFalke)
fa4695da4c build: Sort Makefile.am after renaming file (MarcoFalke)
cccc2784a3 scripted-diff: Move ui_interface to the node lib (MarcoFalke)
fa72ca6a9d qt: Remove unused includes (MarcoFalke)
fac96e6450 wallet: Do not include server symbols (MarcoFalke)
fa0f6c58c1 Revert "Fix link error with --enable-debug" (MarcoFalke)
Pull request description:
This reverts a hacky workaround from commit b83cc0f, which only happens to work due to compiler optimizations. Then, it actually fixes the linker error.
The underlying problem is that the wallet includes symbols from the server (ui_interface), which usually results in linker failures. Though, in this specific case the linker failures have not been observed (unless `-O0`) because our compilers were smart enough to strip unused symbols.
Fix the underlying problem by creating a new header-only with the needed symbol and move ui_interface to node to clarify that this is part of libbitcoin_server.
ACKs for top commit:
Sjors:
ACK faca730
laanwj:
ACK faca73000f
hebasto:
re-ACK faca73000f, since the [previous](https://github.com/bitcoin/bitcoin/pull/19331#pullrequestreview-434420539) review:
Tree-SHA512: e9731f249425aaea50b6db5fc7622e10078cf006721bb87989cac190a2ff224412f6f8a7dd83efd018835302337611f5839e29e15bef366047ed591cef58dfb4
5527be0627 refactor: Add AbortError alias (Hennadii Stepanov)
d924f2a596 Drop MSG_NOPREFIX flag (Hennadii Stepanov)
083daf7fba Pass bilingual_str argument to AbortNode() (Hennadii Stepanov)
d1cca129b4 refactor: Use bilingual_str::empty() (Hennadii Stepanov)
Pull request description:
This PR is a [followup](https://github.com/bitcoin/bitcoin/issues/16218#issuecomment-625919724) of #16224, and it adds `bilingual_str` type argument support to the `AbortNode()` functions.
ACKs for top commit:
MarcoFalke:
ACK 5527be0627👟
Tree-SHA512: bf8b15b14912b1f672e6e588fffa1e6eb6f00b4b23d15d0ced7f18fbdf76919244427feb7217007fe29617049308e13def893a03a87358db819cca9692f59905
b00266fe0c refactor: replace pointers by references within tx_verify.{h,cpp} (Sebastian Falbesoner)
Pull request description:
This PR gets rid of another unnecessary use of raw pointers, similar to PR #19053 (see also issue #19062 where useful commands for finding potential candidates are listed) but in the tx verification module.
For the functions `CalculateSequenceLocks()` and `SequenceLocks()`, the `prevHeights` vector parameter type is changed to be passed as a reference. Note that there were no checks for null pointers -- if one would pass `nullptr` to one of the functions, the following line would immediately lead to a crash:
dcacea096e/src/consensus/tx_verify.cpp (L32)
ACKs for top commit:
Empact:
Code Review ACK b00266fe0c
Tree-SHA512: 0eb71591467905434082029128bdca4df94988c372af40dca325654f6c002c72a00c73776cb5e72d6de2b2f218649211a5dbf19300a2e01f1841d6034e0f01e0
501e6ab4e7 doc: Add documentation for 'checklevel' argument in 'verifychain' RPC call (Calvin Kim)
Pull request description:
Rationale: When ```bitcoin-cli help verifychain``` is called, the user doesn't get any documentation about the ```checklevel``` argument, leading to issues like #18995.
This PR addresses that issue and adds documentation for what each level does, and that each level includes the checks of the previous levels.
ACKs for top commit:
jonatack:
ACK 501e6ab4e7 `git diff 292ed3c 501e6ab` shows only change since last review is the verifychain RPCHelpMan edit; rebuild and retested manually anyway
MarcoFalke:
ACK 501e6ab4e7🚝
Tree-SHA512: 09239f79c25b5c3022b8eb1f76198ba681305d7e8775038e46becffe5f6a14c572e0c5d06b0723fe9d4a015ec42c9f7ca7b80a2a93df0b1b66f5a84a80eeeeb1
ac94141af0 validation: delay flushing undo files in syncing node case (Karl-Johan Alm)
Pull request description:
Fixes#17890. Replaces #17892.
Data files (`{blk|rev}<number>.dat`) pre-allocate space as they are written, and then trims down to the final size once they move on to the next sequence ("finalized flush"). The code currently assumes (incorrectly) that blk and rev files finish at the same time, but because blk files are written as blocks come in, and rev files are written in block height order, rev files end up being written to for awhile after moving on to the next block file, resulting in pre-allocation and waste of up to 1 MB of space per rev file.
The exact point at which rev file writing finishes is the highest height block found inside the corresponding block file, which is already available in the CBlockFileInfo vector. This PR moves finalized flushing of undo files to to directly after the undo data for the previous block file has been written.
There is a branch with annotation that demonstrates how this is handling flushing here: https://github.com/kallewoof/bitcoin/tree/200124-rev-files-annotated
ACKs for top commit:
vasild:
ACK ac94141af (no changes in the code since ed34e00da).
fjahr:
Code review re-ACK ac94141af0
jonatack:
Code review ACK ac94141af0
Tree-SHA512: 1d4e3b3d1d99bd7ebe7a2f632b1231146dd4f9f993c54db3a4090d9c086d95d2e4c327fd936066392b3afc6277b8f3a908d5c5993d4c8e49f72b92a417716dd2
789e9dd3aa validation: use std::chrono in IsCurrentForFeeEstimation() (fanquake)
47be28c8bc validation: use std::chrono in CChainState::FlushStateToDisk() (fanquake)
Pull request description:
Probably up for debate as to which type is used for the constants. Personally, swapping these to hours is more readable.
ACKs for top commit:
MarcoFalke:
ACK 789e9dd3aa
jonatack:
ACK 789e9dd3aa
Tree-SHA512: f4a25cbd00a49a54b7783a1f588be83706dd2a475cecb5c2e8b97b2d4b27c0955a7454d7486f2454e96351c44f233b300c4f4b9ca62fc7336277f10da34dd5c3
ActivateBestChain (ABC) is only called in the "msghand" or one of the
RPC threads, neither of which is a boost::thread. However, ABC is also
called in ThreadImport (which currently happens to be a boost::thread).
In all cases, the interruption_point is redundant with the breakpoint in
ABC that triggers when ShutdownRequested()
VerifyDB is only called in the main thread ("init") or one of the RPC
threads, neither of which is a boost::thread.
152e8baf08 Use salted hasher instead of nonce in sigcache (Jeremy Rubin)
5495fa5850 Add Hash Padding Microbenchmarks (Jeremy Rubin)
Pull request description:
This PR replaces nonces in two places with pre-salted hashers.
The nonce is chosen to be 64 bytes long so that it forces the SHA256 hasher to process the chunk. This leaves the next 64 (or 56 depending if final chunk) open for data. In the case of the script execution cache, this does not make a big performance improvement because the nonce was already properly padded to fit into one buffer, but does make the code a little simpler. In the case of the sig cache, this should reduce the hashing overhead slightly because we are less likely to need an additional processing step.
I haven't benchmarked this, but back of the envelope it should reduce the hashing by one buffer for all combinations except compressed public keys with compact signatures.
ACKs for top commit:
ryanofsky:
Code review ACK 152e8baf08. No code changes, just rebase since last review and expanded commit message
Tree-SHA512: b133e902fd595cfe3b54ad8814b823f4d132cb2c358c89158842ae27daee56ab5f70cde2585078deb46f77a6e7b35b4cc6bba47b65302b7befc2cff254bad93d
0ae8f18dfe build: add -Wgnu to compile flags (fanquake)
3a0fd7726b Remove use of non-standard zero variadic macros (Ben Woosley)
49f6178c3e Drop unused LOG_TIME_MICROS helper (Ben Woosley)
5d4999951e prevector: Avoid unnamed struct, which is a GNU extension (DesWurstes)
Pull request description:
Since we [started using](https://github.com/bitcoin/bitcoin/pull/7165) the `ax_cxx_compile_stdcxx.m4` macro we've been passing `[noext]` to indicate that we don't want to use an extended mode, i.e GNU extensions. Speaking to Cory he clarified that the intention was to "require only vanilla c++11 and turn _off_ extension support so they would fail to compile".
However in the codebase we are currently making use of some GNU extensions. We should either remove there usage, or at least amend our CXX compiler checks. I'd prefer the former.
#### anonymous structs
```bash
./prevector.h:153:9: warning: anonymous structs are a GNU extension [-Wgnu-anonymous-struct]
struct {
```
This is fixed in b849212c1e.
#### variadic macros
```bash
./undo.h:57:50: warning: must specify at least one argument for '...' parameter of variadic macro [-Wgnu-zero-variadic-macro-arguments]
::Unserialize(s, VARINT(nVersionDummy));
```
This is taken care of in #18087.
The `LOG_TIME_*` macros introduced in #16805 make use of a [GNU extension](https://gcc.gnu.org/onlinedocs/cpp/Variadic-Macros.html).
```bash
In file included from validation.cpp:22:
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
BCLog::Timer<std::chrono::milliseconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
^
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:99:99: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
./logging/timer.h:101:92: warning: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Wgnu-zero-variadic-macro-arguments]
BCLog::Timer<std::chrono::seconds> PASTE2(logging_timer, __COUNTER__)(__func__, end_msg, ## __VA_ARGS__)
^
6 warnings generated.
```
This is fixed in 081a0ab64eb442bc85c4d4a4d3bc2c8e97ac2a6d and 612e8e138b97fc5ad2f38847300132a8fc423c3f.
#### prevention
To ensure that usage doesn't creep back in we can add [`-Wgnu`](https://clang.llvm.org/docs/DiagnosticsReference.html#wgnu) to our compile time flags, which will make Clang warn whenever it encounters GNU extensions.
This would close#14130.
Also related to #17230, where it's suggested we use a GNU extension, the `gnu::pure` attribute.
ACKs for top commit:
practicalswift:
ACK 0ae8f18dfe -- diff looks correct
MarcoFalke:
ACK 0ae8f18dfe
vasild:
utACK 0ae8f18df
dongcarl:
ACK 0ae8f18dfe
Tree-SHA512: c517404681ef8edf04c785731d26105bac9f3c9c958605aa24cbe399c649e7c5ee0c4aa8e714fd2b2d335e2fbea4d571e09b0dec36678ef871f0a6683ba6bb7f
faec3dc2ad init: Remove boost from ThreadImport (MarcoFalke)
Pull request description:
Can be tested by calling `-reindex` or `-loadblock` and then pressing `CTRL`+`C`.
Should print something like:
```
...
2020-04-27T19:34:31Z [loadblk] Reindexing block file blk00005.dat...
^C2020-04-27T19:34:32Z [loadblk] Shutdown requested. Exit ThreadImport
2020-04-27T19:34:32Z [qt-init] Interrupting HTTP server
...
```
ACKs for top commit:
laanwj:
Code review ACK faec3dc2ad
hebasto:
ACK faec3dc2ad, tested on Linux Mint 19.3 (x86_64) both `bitcoind` and `bitcoin-qt` binaries.
Tree-SHA512: e105af18d98296d82ec99f48e478cf44577e3c32f7e4b47617a7bc7cbf71d6becb92722f229a1be38d58ad29712704509ad9740d8ab8cd3104cf90057664b437
692f8307fc test: add test for witness commitment index (fanquake)
06442549f8 validation: Add minimum witness commitment size constant (fanquake)
Pull request description:
16101de5f3: Per [BIP 141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Commitment_structure), the witness commitment structure is at least 38 bytes,
OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte
SHA256 hash. It can be longer, however any additional data has no
consensus meaning.
54f8c48d6a: As per BIP 141, if there is more than 1 pubkey that matches the witness
commitment structure, the one with the highest output index should be
chosen. This adds a sanity check that we are doing that, which will fail
if anyone tries to "optimize" GetWitnessCommitmentIndex() by returning
early.
ACKs for top commit:
MarcoFalke:
ACK 692f8307fc🌵
jonatack:
Code review ACK 692f830
ajtowns:
ACK 692f8307fc
jnewbery:
utACK 692f8307fc
laanwj:
ACK 692f8307fc
Tree-SHA512: 7af3fe4b8a52fea2cdd0aec95f7bb935351a77b73d934bc88d6625a3503311b2a062cba5190b2228f97caa76840db3889032d910fc8e318ca8e7810a8afbafa0
Use salted hasher instead of nonce in Script Execution Cache
Don't read more than 32 bytes from GetRand
Apply g_* naming convention to scriptExecutionCache in validation.cpp
Fully apply g_* naming convention to scriptCacheHasher
Write same uint256 nonce twice for cache hash rather than calling getrand twice
Use salted hasher instead of nonce in sigcache
Use salted hasher instead of nonce in Script Execution Cache
Don't read more than 32 bytes from GetRand
Apply g_* naming convention to scriptExecutionCache in validation.cpp
Fully apply g_* naming convention to scriptCacheHasher
Write same uint256 nonce twice for cache hash rather than calling getrand twice
50fc4df6c4 [mempool] Persist unbroadcast set to mempool.dat (Amiti Uttarwar)
297a178536 [test] Integration tests for unbroadcast functionality (Amiti Uttarwar)
6851502472 [refactor/test] Extract P2PTxInvStore into test framework (Amiti Uttarwar)
dc1da48dc5 [wallet] Update the rebroadcast frequency to be ~1/day. (Amiti Uttarwar)
e25e42f20a [p2p] Reattempt initial send of unbroadcast transactions (Amiti Uttarwar)
7e93eecce3 [util] Add method that returns random time in milliseconds (Amiti Uttarwar)
89eeb4a333 [mempool] Track "unbroadcast" transactions (Amiti Uttarwar)
Pull request description:
This PR introduces mempool tracking of unbroadcast transactions and periodic reattempts at initial broadcast. This is a part of the rebroadcast project, and a standalone privacy win.
The current rebroadcast logic is terrible for privacy because 1. only the source wallet rebroadcasts transactions and 2. it does so quite frequently. In the current system, if a user submits a transaction that does not immediately get broadcast to the network (eg. they are offline), this "rebroadcast" behavior is the safety net that can actually serve as the initial broadcast. So, keeping the attempts frequent is important for initial delivery within a reasonable timespan.
This PR aims to improve # 2 by reducing the wallet rebroadcast frequency to ~1/day from ~1/15 min. It achieves this by separating the notion of initial broadcast from rebroadcasts. With these changes, the mempool tracks locally submitted transactions & periodically reattempts initial broadcast. Transactions submitted via the wallet or RPC are added to an "unbroadcast" set & are removed when a peer sends a `getdata` request, or the transaction is removed from the mempool. Every 10-15 minutes, the node reattempts an initial broadcast. This enables reducing the wallet rebroadcast frequency while ensuring the transactions will be propagated to the network.
For privacy improvements around # 1, please see #16698.
Thank you to gmaxwell for the idea of how to break out this subset of functionality (https://github.com/bitcoin/bitcoin/pull/16698#issuecomment-571399346)
ACKs for top commit:
fjahr:
Code review ACK 50fc4df6c4
MarcoFalke:
ACK 50fc4df6c4, I think this is ready for merge now 👻
amitiuttarwar:
The current tip `50fc4df` currently has 6 ACKs on it, so I've opened #18807 to address the last bits.
jnewbery:
utACK 50fc4df6c4.
ariard:
Code Review ACK 50fc4df (minor points no need to invalid other ACKs)
robot-visions:
ACK 50fc4df6c4
sipa:
utACK 50fc4df6c4
naumenkogs:
utACK 50fc4df
Tree-SHA512: 2dd935d645d5e209f8abf87bfaa3ef0e4492705ce7e89ea64279cb27ffd37f4727fa94ad62d41be331177332f8edbebf3c7f4972f8cda10dd951b80a28ab3c0f
Per BIP 141, the witness commitment structure is atleast 38 bytes,
OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte
SHA256 hash. It can be longer, however any additional data has no
consensus meaning.
21fa0a44ab [docs] use consistent naming for possible_overwrite (John Newbery)
2685c214cc [tests] small whitespace fixup (John Newbery)
e9936966c0 scripted-diff: Rename PRUNED to SPENT in coins tests (John Newbery)
c205979031 [docs] Improve commenting in coins.cpp|h (John Newbery)
Pull request description:
- Add full commenting for spentness / DIRTYness / FRESHness and which combinations are valid
- Remove the 'pruned' terminology, which doesn't make sense since per-txout chainstate db was merged (#10195).
- Rename `potential_overwrite` to `possible_overwrite` to standardize terminology (there were previously examples of both, which made searching the codebase difficult).
- Make other minor improvements to the comments
ACKs for top commit:
jonatack:
Re-ACK 21fa0a4 per `git diff 98bee55 21fa0a4` the only change since my previous review is the following code commenting diff in `src/coins.cpp::L177-179`; rebuilt/ran unit tests anyway as a sanity check on the unit test changes.
Tree-SHA512: 391e01588ef5edb417250080cec17361f982c4454bc5f8c6d78bbd528c68a2bb94373297760691295c24660ce1022ad3ef7599762f736c8eed772ce096d38c3d
f63dec189c [REFACTOR] Initialize PrecomputedTransactionData in CheckInputScripts (Pieter Wuille)
Pull request description:
This is a single commit taken from the Schnorr/Taproot PR #17977.
Add a default constructor to `PrecomputedTransactionData`, which doesn't initialize the struct's members. Instead they're initialized inside the `CheckInputScripts()` function. This allows a later commit to add the spent UTXOs to that structure. The spent UTXOs are required for the schnorr signature hash, since it commits to the scriptPubKeys. See https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#common-signature-message for details.
By itself, this isn't really an improvement to the code, but I think it makes sense to separate out the refactor/moveonly commits from PR #17977 so that PR is only the logical changes needed for Schnorr/Taproot.
ACKs for top commit:
jonatack:
Re-ACK f63dec1 `git diff 851908d f63dec1` shows no change since last ACK.
sipa:
utACK f63dec189c
theStack:
re-ACK f63dec189c
fjahr:
Re-ACK f63dec189c
ariard:
Code Review ACK f63dec1
Tree-SHA512: ecf9154077824ae4c274b4341e985797f3648c0cb0c31cb25ce382163b923a3acbc7048683720be4ae3663501801129cd0f48c441a36f049cc304ebe9f30994e
Add a default constructor to `PrecomputedTransactionData`, which doesn't
initialize the struct's members. Instead they're initialized inside the
`CheckInputScripts()` function. This allows a later commit to add the
spent UTXOs to that structure.
c9017ce3bc protect g_chainman with cs_main (James O'Beirne)
2b081c4568 test: add basic tests for ChainstateManager (James O'Beirne)
4ae29f5f0c use ChainstateManager to initialize chainstate (James O'Beirne)
5b690f0aae refactor: move RewindBlockIndex to CChainState (James O'Beirne)
89cdf4d569 validation: introduce unused ChainstateManager (James O'Beirne)
8e2ecfe249 validation: add CChainState.m_from_snapshot_blockhash (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
This changeset introduces `ChainstateManager`, which is responsible for creating and managing access to multiple chainstates. Until we allow chainstate creation from UTXO snapshots (next assumeutxo PR?) it's basically unnecessary, but it is a prerequisite for background IBD support.
Changes are also made to the initialization process to make use of `g_chainman` and thus clear the way for multiple chainstates being loaded on startup.
One immediate benefit of this change is that we no longer have the `g_blockman` global, but instead have the ChainstateManager inject a reference of its shared BlockManager into any chainstate it creates.
Another immediate benefit is that uses of `ChainActive()` and `ChainstateActive()` are now covered by lock annotations. Because use of `g_chainman` is annotated to require cs_main, these two functions subsequently follow.
Because of whitespace changes, this diff looks bigger than it is. E.g., 4813167d98 is most easily reviewed with
```sh
git show --color-moved=dimmed_zebra -w 4813167d98
```
ACKs for top commit:
MarcoFalke:
re-ACK c9017ce3bc📙
fjahr:
Code Review Re-ACK c9017ce3bc
ariard:
Code Review ACK c9017ce
ryanofsky:
Code review ACK c9017ce3bc. No changes since last review other than a straight rebase
Tree-SHA512: 3f250d0dc95d4bfd70852ef1e39e081a4a9b71a4453f276e6d474c2ae06ad6ae6a32b4173084fe499e1e9af72dd9007f4a8a375c63ce9ac472ffeaada41ab508
Data files are pre-allocated, and upon flush/finalization, they are trimmed down to their resulting size.
Block (blk) files are written to disk as blocks come in, which is often out of order, whereas undo (rev) files are written sequentially, as each block is added to the top of the chain.
When a block file hits the size limit, the system flushes and trims the file down to its final size, and moves on to the next block file.
Case 1: blocks are added to the chain as they come in (synced up node case) -- in this case, we will flush and finalize the undo file together with the block file.
Case 2: blocks are added to the chain after they have been downloaded (syncing node case) -- in this case, we postpone finalizing the undo file until we know the undo data for the last block in the file has been written to disk.
e57980b473 [mempool] Remove NotifyEntryAdded and NotifyEntryRemoved callbacks (John Newbery)
2dd561f361 [validation] Remove pool member from ConnectTrace (John Newbery)
969b65f3f5 [validation] Remove NotifyEntryRemoved callback from ConnectTrace (John Newbery)
5613f9842b [validation] Remove conflictedTxs from PerBlockConnectTrace (John Newbery)
cdb893443c [validation interface] Remove vtxConflicted from BlockConnected (John Newbery)
1168394d75 [wallet] Notify conflicted transactions in TransactionRemovedFromMempool (John Newbery)
Pull request description:
These boost signals were added in #9371, before we had a `TransactionRemovedFromMempool` method in the validation interface. The `NotifyEntryAdded` callback was used by validation to build a vector of conflicted transactions when connecting a block, which the wallet was notified of in the `BlockConnected` CValidationInterface callback.
Now that we have a `TransactionRemovedFromMempool` callback, we can fire that signal directly from the mempool for conflicted transactions.
Note that #9371 was implemented to ensure `-walletnotify` events were fired for these conflicted transaction. We inadvertently stopped sending these notifications in #16624 (Sep 2019 commit 7e89994). We should probably fix that, but in a different PR.
ACKs for top commit:
jonatack:
Re-ACK e57980b
ryanofsky:
Code review ACK e57980b473, no code changes since previous review, but helpful new code comments have been added and the PR description is now more clear about where the old code came from
Tree-SHA512: 3bdbaf1ef2731e788462d4756e69c42a1efdcf168691ce1bbfdaa4b7b55ac3c5b1fd4ab7b90bcdec653703600501b4224d252cfc086aef28f9ce0da3b0563a69
ec30a79f1c Fix UB with bench on genesis block (Gregory Sanders)
Pull request description:
During the loading of the genesis block, the bench print lines in ConnectTip divide by zero due to early return in ConnectBlock.
ACKs for top commit:
practicalswift:
ACK ec30a79f1c
sipa:
utACK ec30a79f1c
promag:
ACK ec30a79, `nBlocksTotal` is only used in logging.
Tree-SHA512: b3bdbb58d10d002a2293d7f99196b227ed9f4ca8c6cd08981e95cc964be47efed98b91fad276ee6da5cf7e6684610998ace7ce9bace172dd6c51c386d985b83c
I'd previously attempted to create a specialized lock for ChainstateManager,
but it turns out that because that lock would be required for functions like
ChainActive() and ChainstateActive(), it created irreconcilable lock inversions
since those functions are used so broadly throughout the codebase.
Instead, I'm just using cs_main to protect the contents of g_chainman.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
ChainstateManager is responsible for creating and managing multiple
chainstates, and will provide a high-level interface for accessing the
appropriate chainstate based upon a certain use.
Incorporates feedback from Marco Falke. Additional documentation written
by Russ Yanofsky.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
This parameter is unused, but in future commits will allow ChainstateManager to
differentiate between chainstates created from a UTXO snapshot from those that
weren't.
This replaces one remaining instance of the literal "BTC" string with
the CURRENCY_UNIT constant, as is done in most of the codebase already.
The other remaining instance (which is just part of a log message and thus
not really user-visible) is just removed.
After this change, no instance of literal "BTC" remains anywhere in the
non-Qt and non-test codebase.
ConnectTrace used to subscribe to the mempool's NotifyEntryRemoved
callback to be notified of transactions removed for conflict. Since
PerBlockConnectTrace no longer tracks conflicted transactions,
ConnectTrace no longer requires these notifications.
