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
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
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.
0a50019fde Walk pindexBestHeader back to ChainActive().Tip() if it is invalid (Matt Corallo)
Pull request description:
Instead of keeping pindexBestHeader set to the best header we've
ever seen, reset it back to our validated tip if we find an ancestor
of it turns out to be invalid. While the name is now a bit confusing,
this matches much better with how it is used in practice, see below.
Further, this opens up more use-cases for it in the future, namely
aggressively searching for new peers in case we have discovered
(possibly via some covert channel) headers which we do not know to be
invalid, but which we cannot find block data for.
Places pindexBestHeader is used:
* Various GUI displays of the best header and getblockchaininfo["headers"],
I don't think changing this is bad, and if anything this is less confusing
in the presence of an invalid block.
* IsCurrentForFeeEstimation(): If anything I think ensuring pindexBestHeader
isn't some crazy invalid chain is better than the alternative, even in the
case where you are rejecting the current chain due to hardware error (since
hopefully in that case you won't get any new blocks anyway).
* ConnectBlock assumevalid checks: We use pindexBestHeader to check that the
block we're connecting leads to something with nMinimumChainWork (preventing
a user-set assumevalid from having bogus work) and that the block we're
connecting leads to pindexBestHeader (I'm not too worried about this one -
it's nice to "disable" assumevalid if we have a long invalid headers chain,
but I don't see it as a critical protection).
* BlockRequestAllowed() uses pindexBestHeader as its target to ensure the
requested block is within a month of the "current chain". I don't think this
is a meaningful difference, if we're rejecting the current tip we're
trivially fingerprintable anyway, and if the chain really does have a bunch
of invalid crap near the tip, using the best not-invalid header is likely a
better criteria.
* ProcessGetBlockData uses pindexBestHeader as the "current chain" definition
of whether a block request is "historical" for the purpose of bandwidth
limiting. Similarly, I don't see why this is a meaningful change.
* We use pindexBestHeader for requesting missing headers on receipt of a
headers/compact block message or block inv as well as for initial getheaders.
I think this is definitely wrong, using the best not-invalid header for such
requests is much better.
* We use pindexBestHeader to define the "current chain" for deciding when
we're close to done with initial headers sync. I don't think this is a
meaningful change.
* We use pindexBestHeader to decide if initial headers sync has timed out. If
we're rejecting the chain due to hardware error this may result in
additional cases where we ban a peer, but this is already true, so I think
its fine.
ACKs for top commit:
fjahr:
ACK 0a50019fde
kallewoof:
ACK 0a50019fde
ariard:
utACK 0a50019
Tree-SHA512: 2ecfa973a9878a00313ae7ede94a9bd7710e0caf55b544b10bbc46dc463a0478cbaf477e6cdd072356d5a0c5fb3848e9339284af785a2995c20bae8bd23f23e5
-BEGIN VERIFY SCRIPT-
# Delete outdated alias for RecursiveMutex
sed -i -e '/CCriticalSection/d' ./src/sync.h
# Replace use of outdated alias with RecursiveMutex
sed -i -e 's/CCriticalSection/RecursiveMutex/g' $(git grep -l CCriticalSection)
-END VERIFY SCRIPT-
02b9511d6b tests: add tests for GetCoinsCacheSizeState (James O'Beirne)
b17e91d842 refactoring: introduce CChainState::GetCoinsCacheSizeState (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 pulls out the routine for detection of how full the coins cache is from
FlushStateToDisk. We use this logic independently when deciding when to flush
the coins cache during UTXO snapshot activation ([see here](231fb5f17e (diff-24efdb00bfbe56b140fb006b562cc70bR5275))).
ACKs for top commit:
ariard:
Code review ACK 02b9511.
ryanofsky:
Code review ACK 02b9511d6b. Just rebase, new COIN_SIZE comment, and new test message since last review
Tree-SHA512: 8bdd78bf68a4a5d33a776e73fcc2857f050d6d102caa4997ed19ca25468c1358e6e728199d61b423033c02e6bc8f00a1d9da52cf17a2d37d70860fca9237ea7c
3bd8db80d8 [validation] fix comments in CheckInputScripts() (John Newbery)
6f6465cefc scripted-diff: [validation] Rename CheckInputs to CheckInputScripts (John Newbery)
Pull request description:
CheckInputs() used to check no double spends, scripts & sigs and amounts. Since
832e074, the double spend and amount checks
have been moved to CheckTxInputs(), and CheckInputs() now just validates
input scripts. Rename the function to CheckInputScripts().
Also fix incorrect comments.
ACKs for top commit:
MarcoFalke:
re-ACK 3bd8db80d8, did the rebase myself, checked the scripted diff 👡
promag:
ACK 3bd8db80d8 :trollface:
Tree-SHA512: 7b3f8597d210492798fb784ee8ea47ea6377519111190161c7cc34a967509013f4337304f52e9bedc97b7710de7b0ff8880e08cd7f867754567f82e7b02c794c
This separates out some logic for detecting how full the coins cache is from
FlushStateToDisk. We'll want to reuse this logic when deciding when to flush
the coins cache during UTXO snapshot activation.
Commit d449772cf6 stopped setting
COINBASE_FLAGS, and it looks like it hasn't been used since P2SH.
Update the help string to remove "flags", which is not specified in
BIP 22.
2f5f7d6b13 GuessVerificationProgress: cap the ratio to 1 (darosior)
Pull request description:
Noticed `getblockchaininfo` would return a `verificationprogress` > 1, especially while generating. This caps the verification progress to `1`.
Tried to append a check to functional tests but this would pass even without the patch, so it seems better to not add a superfluous check (but this can easily be reproduced by trying to generate blocks in the background and `watch`ing `getblockchainfo`).
ACKs for top commit:
laanwj:
ACK 2f5f7d6b13
promag:
ACK 2f5f7d6b13.
Tree-SHA512: fa3aca12acab9c14dab3b2cc94351082f548ea6e6c588987cd86e928a00feb023e8112433658a0e85084e294bfd940eaafa33fb46c4add94146a0901bc1c4f80
36b68de5b2 Remove getBlockDepth method from Chain::interface (Antoine Riard)
b66c429c56 Remove locked_chain from GetDepthInMainChain and its callers (Antoine Riard)
0ff03871ad Use CWallet::m_last_block_processed_height in GetDepthInMainChain (Antoine Riard)
f77b1de16f Only return early from BlockUntilSyncedToCurrentChain if current tip is exact match (Antoine Riard)
769ff05e48 Refactor some importprunedfunds checks with guard clause (Antoine Riard)
5971d3848e Add block_height field in struct Confirmation (Antoine Riard)
9700fcb47f Replace CWalletTx::SetConf by Confirmation initialization list (Antoine Riard)
5aacc3eff1 Add m_last_block_processed_height field in CWallet (Antoine Riard)
10b4729e33 Pass block height in Chain::BlockConnected/Chain::BlockDisconnected (Antoine Riard)
Pull request description:
Work starter to remove Chain::Lock interface by adding m_last_block_processed_height in CWallet and m_block_height in CMerkleTx to avoid GetDepthInMainChain having to keep a lock . Once this one done, it should ease work to wipe out more cs_main locks from wallet code.
I think it's ready for a first round of review before to get further.
- `BlockUntilSyncedToCurrent` : restrain isPotentialTip to isTip because we want to be sure that wallet see BlockDisconnected callbacks if its height differs from the Chain one. It means during a reorg, an RPC could return before the BlockDisconnected callback had been triggered. This could cause a tx that had been included in the disconnected block to be displayed as confirmed, for example.
~~- `AbandonTransaction` : in case of conflicted tx (nIndex = -1), we set its m_block_height to the one of conflicting blocks, but if this height is superior to CWallet::m_last_block_processed_height, that means tx isn't conflicted anymore so we return 0 as tx is again unconfirmed~~ After #16624, we instead rely on Confirmation.
~~- `AddToWalletIfInvolvingMe`: in case of block disconnected, transactions are added to mempool again, so we need to replace old txn in `mapWallet` with a height set to zero so we remove check on block_hash.IsNull~~ Already done in #16624
ACKs for top commit:
jnewbery:
@jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch (36b68de5b2).
jkczyz:
> @jkczyz you've ACKed an intermediate commit (github annoyingly orders commits in date order, not commit order). Did you mean to ACK the final commit in this branch ([36b68de](36b68de5b2)).
meshcollider:
utACK 36b68de5b2
ryanofsky:
Code review ACK 36b68de5b2. Changes since last review: new jkczyz refactor importprunedfunds commit, changed BlockUntilSyncedToCurrentChainChanges commit title and description, changed Confirmation struct field order and line-wrapped comment
jnewbery:
utACK 36b68de5b2
promag:
Code review ACK 36b68de5b2.
Tree-SHA512: 08b89a0bcc39f67c82a6cb6aee195e6a11697770c788ba737b90986b4893f44e90d1ab9ef87239ea3766508b7e24ea882b7199df41173ab27a3d000328c14644
CheckInputs() used to check no double spends, scripts & sigs and amounts. Since
832e0744cb, the double spend and amount checks
have been moved to CheckTxInputs(), and CheckInputs() now just validates
input scripts. Rename the function to CheckInputScripts().
-BEGIN VERIFY SCRIPT-
sed -i -E -e 's/CheckInputs\b/CheckInputScripts/g' $(git grep -l CheckInputs | grep -v doc/)
-END VERIFY SCRIPT-
5506ecfe7a [refactor] Replace global int nScriptCheckThreads with bool (John Newbery)
d9957623b4 [tests] Don't use TestingSetup in the checkqueue_tests (John Newbery)
Pull request description:
The meaning of this value is confusing. Refactor it and add comments.
ACKs for top commit:
sipa:
ACK 5506ecfe7a
promag:
ACK 5506ecfe7a, only change was addressing my nits.
laanwj:
Code review ACK 5506ecfe7a
MarcoFalke:
ACK 5506ecfe7a🥐
Tree-SHA512: 78536727c98d2c23f3c0f3f169131474fef9a4486ae65029011caf06eab30f6f70ff73a65b2fb04a5d969fc1150858d1c6ea4767f04d48c1eea6b829316d0e63
The global nScriptCheckThreads int is confusing and is only needed for
its int-ness in AppInitMain. Move all `-par` parsing logic there and
replace the int nScriptCheckThreads with a bool
g_parallel_script_checks.
Also tidy up logic and improve comments.
dcef9a2922 logs: add timing information to FlushStateToDisk() (James O'Beirne)
41edaf227a logs: add BCLog::Timer and related macros (James O'Beirne)
Pull request description:
It's currently annoying to detect FlushStateToDisk() calls when benchmarking since they have to be inferred from a drop in coins count from the `UpdateTip: ` log messages. This adds a new logging utility, `BCLog::Timer`, and some related macros that are generally useful for printing timing-related logging messages, and a message that is unconditionally written when the coins cache is flushed to disk.
```
2019-09-04T20:17:51Z FlushStateToDisk: write block and undo data to disk completed (3ms)
2019-09-04T20:17:51Z FlushStateToDisk: write block index to disk completed (370ms)
2019-09-04T20:17:51Z FlushStateToDisk: write coins cache to disk (2068451 coins, 294967kB) completed (21481ms)
```
ACKs for top commit:
laanwj:
Thanks, ACK dcef9a2922
ryanofsky:
Code review ACK dcef9a2922. No changes since last review other than moving code to new timer.h header
Tree-SHA512: 6d61e48a062d3edb48d0e056a6f0b1f8031773cc99289ee4544f8349d24526b88519e1e304009d56e428f1eaf76c857bf8e7e1c0b6873a6f270306accb5edc3d
To do so we update CValidationInterface::BlockDisconnect to take a
CBlockIndex pointing to the block being disconnected.
This new parameter will be use in the following commit to establish
wallet height.
Instead of keeping pindexBestHeader set to the best header we've
ever seen, reset it back to our validated tip if we find an ancestor
of it turns out to be invalid. While the name is now a bit confusing,
this matches much better with how it is used in practice, see below.
Further, this opens up more use-cases for it in the future, namely
aggressively searching for new peers in case we have discovered
(possibly via some covert channel) headers which we do not know to be
invalid, but which we cannot find block data for.
Places pindexBestHeader is used:
* Various GUI displays of the best header and getblockchaininfo["headers"],
I don't think changing this is bad, and if anything this is less confusing
in the presence of an invalid block.
* IsCurrentForFeeEstimation(): If anything I think ensuring pindexBestHeader
isn't some crazy invalid chain is better than the alternative, even in the
case where you are rejecting the current chain due to hardware error (since
hopefully in that case you won't get any new blocks anyway).
* ConnectBlock assumevalid checks: We use pindexBestHeader to check that the
block we're connecting leads to something with nMinimumChainWork (preventing
a user-set assumevalid from having bogus work) and that the block we're
connecting leads to pindexBestHeader (I'm not too worried about this one -
it's nice to "disable" assumevalid if we have a long invalid headers chain,
but I don't see it as a critical protection).
* BlockRequestAllowed() uses pindexBestHeader as its target to ensure the
requested block is within a month of the "current chain". I don't think this
is a meaningful difference, if we're rejecting the current tip we're
trivially fingerprintable anyway, and if the chain really does have a bunch
of invalid crap near the tip, using the best not-invalid header is likely a
better criteria.
* ProcessGetBlockData uses pindexBestHeader as the "current chain" definition
of whether a block request is "historical" for the purpose of bandwidth
limiting. Similarly, I don't see why this is a meaningful change.
* We use pindexBestHeader for requesting missing headers on receipt of a
headers/compact block message or block inv as well as for initial getheaders.
I think this is definitely wrong, using the best not-invalid header for such
requests is much better.
* We use pindexBestHeader to define the "current chain" for deciding when
we're close to done with initial headers sync. I don't think this is a
meaningful change.
* We use pindexBestHeader to decide if initial headers sync has timed out. If
we're rejecting the chain due to hardware error this may result in
additional cases where we ban a peer, but this is already true, so I think
its fine.
