a720b92 Remove includes in .cpp files for things the corresponding .h file already included (practicalswift)
Pull request description:
Remove includes in .cpp files for things the corresponding .h file already included.
Example case:
* `addrdb.cpp` includes `addrdb.h` and `fs.h`
* `addrdb.h` includes `fs.h`
Then remove the direct inclusion of `fs.h` in `addrman.cpp` and rely on the indirect inclusion of `fs.h` via the included `addrdb.h`.
In line with the header include guideline (see #10575).
Tree-SHA512: 8704b9de3011a4c234db336a39f7d2c139e741cf0f7aef08a5d3e05197e1e18286b863fdab25ae9638af4ff86b3d52e5cab9eed66bfa2476063aa5c79f9b0346
22fddde Avoid calling GetSerializeSize on each tx in a block if !fTxIndex (Matt Corallo)
2862aca Move some additional variables into CChainState private (Matt Corallo)
fd4d80a Create initial CChainState to hold chain state information (Matt Corallo)
e104f0f Move block writing out of AcceptBlock (Matt Corallo)
50701ba Move txindex/undo data disk location stuff out of ConnectBlock (Matt Corallo)
93a34cf Make DisconnectBlock unaware of where undo data resides on disk (Matt Corallo)
Pull request description:
CChainState should eventually, essentially, be our exposed "libconsensus", but we're probably a few releases away, so the real goal is to clarify our internal interfaces. The main split was a big step, but validation.cpp is still a somewhat ranomly-mixed bag of functions that are pure functions which validate inputs (which should probably either merge with their callers or move into another file in consensus/), read/write data from disk, manipulate our current chain state (which moves into CChainState), and do mempool transaction validation.
Obviously this is only a small step, but some effort is made to clean up what functions the functions in CChainState call, and obviously as things are added its easy to keep clear "CChainState::* cannot call anything except via callbacks through CValidationInterface, pure functions, or disk read/write things". Right now there are some glaring violations in mempool callbacks, and general flushing logic needs cleaning up (FlushStateToDisk maybe shouldnt be called, and there should be an API towards setDirtyBlockIndex, but I'll leave that for after @sipa's current changesets land).
Tree-SHA512: 69b8ec191b36b19c9492b4dee74c8057621fb6ec98ad219e8da0b2ed5c3ad711b10b5af9ff1117e8807ccf88918eeeab573be8448baecc9a59f099c53095985b
434526a [test] Add tests for getrawtransaction with block hash. (Karl-Johan Alm)
b167951 [rpc] Allow getrawtransaction to take optional blockhash to fetch transaction from a block directly. (Karl-Johan Alm)
a5f5a2c [rpc] Fix fVerbose parsing (remove excess if cases). (Karl-Johan Alm)
Pull request description:
[Reviewer hint: use [?w=1](https://github.com/bitcoin/bitcoin/pull/10275/files?w=1) to avoid seeing a bunch of indentation changes.]
Presuming a user knows the block hash of the block containing a given transaction, this PR allows them to fetch the raw transaction, even without `-txindex`. It also enables support for getting transactions that are in orphaned blocks.
Note that supplying a block hash will override mempool and txindex support in `GetTransaction`. The rationale behind this is that a transaction may be in multiple places (orphaned blocks) and if the user supplies an explicit block hash it should be adhered to.
```Bash
$ # a41.. is a tx inside an orphan block ..3c6f.. -- first try getting it normally
$ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1
error code: -5
error message:
No such mempool transaction. Use -txindex to enable blockchain transaction queries. Use gettransaction for wallet transactions.
$ # now try with block hash
$ ./bitcoin-cli getrawtransaction a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79 1 0000000000000000003c6fe479122bfa4a9187493937af1734e1e5cd9f198ec7
{
"hex": "01000000014e7e81144e42f6d65550e59b715d470c9301fd7ac189[...]90488ac00000000",
"inMainChain": false,
"txid": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79",
"hash": "a41e66ee1341aa9fb9475b98cfdc1fe1261faa56c0a49254f33065ec90f7cd79",
"size": 225,
[...]
}
$ # another tx 6c66... in block 462000
$ ./bitcoin-cli getrawtransaction 6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735 1 00000000000000000217f2c12922e321f6d4aa933ce88005a9a493c503054a40
{
"hex": "0200000004d157[...]88acaf0c0700",
"inMainChain": true,
"txid": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735",
"hash": "6c66b98191e9d6cc671f6817142152ebf6c5cab2ef008397b5a71ac13255a735",
"size": 666,
[...]
}
$
```
Tree-SHA512: 279be3818141edd3cc194a9ee65929331920afb30297ab2d6da07293a2d7311afee5c8b00c6457477d9f1f86e86786a9b56878ea3ee19fa2629b829d042d0cda
680bc2cbb Use range-based for loops (C++11) when looping over map elements (practicalswift)
Pull request description:
Before this commit:
```c++
for (std::map<T1, T2>::iterator x = y.begin(); x != y.end(); ++x) {
T1 z = (*x).first;
…
}
```
After this commit:
```c++
for (auto& x : y) {
T1 z = x.first;
…
}
```
Tree-SHA512: 954b136b7f5e6df09f39248a6b530fd9baa9ab59d7c2c7eb369fd4afbb591b7a52c92ee25f87f1745f47b41d6828b7abfd395b43daf84a55b4e6a3d45015e3a0
9d811dc Document partial validation in ConnectBlock() (Suhas Daftuar)
Pull request description:
`ConnectBlock()` relies on validation that happens in `ContextualCheckBlock()` and
`ContextualCheckBlockHeader()`. This has implications for implementing consensus
changes and handling software upgrade to ensure that nodes upgrading their
software end up enforcing all the consensus rules.
Tree-SHA512: 36a252af2221b0e5d5d6f8d5f8b16f8b566ca0db2d56242130a5523302c8757599ac234594a6a946c1689b260d18a32c2c7f8c3831304e78b9832e2ce5ac435a
ConnectBlock() relies on validation that happens in ContextualCheckBlock() and
ContextualCheckBlockHeader(). This has implications for implementing consensus
changes and handling software upgrade to ensure that nodes upgrading their
software end up enforcing all the consensus rules.
89f0312 Remove redundant pwallet nullptr check (Matt Corallo)
c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo)
3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo)
cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo)
e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo)
17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo)
5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo)
5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo)
0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo)
2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo)
0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo)
a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo)
Pull request description:
Based on #10179, this effectively reverts #9583, regaining most of the original speedups of #7946.
This concludes the work of #9725, #10178, and #10179.
See individual commit messages for more information.
Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9
d61845818 Have SegWit active by default (Pieter Wuille)
4bd89210a Unit tests for always-active versionbits. (Anthony Towns)
d07ee77ab Always-active versionbits support (Pieter Wuille)
18e071841 [consensus] Pin P2SH activation to block 173805 on mainnet (John Newbery)
526023aa7 Improve handling of BIP9Deployment limits (Anthony Towns)
Pull request description:
Most tests shouldn't have to deal with the now-historical SegWit activation transition (and other deployments, but SegWit is certainly the hardest one to accomodate).
This PR makes a versionbits starttime of -1 equal to "always active", and enables it by default for SegWit on regtest. Individual tests can override this by using the existing `-vbparams` option.
A few unit tests and functional tests are adapted to indeed override vbparams, as they specifically test the transition.
This is in preparation for wallet SegWit support, but I thought having earlier eyes on it would be useful.
Tree-SHA512: 3f07a7b41cf46476e6c7a5c43244e68c9f41d223482cedaa4c02a3a7b7cd0e90cbd06b84a1f3704620559636a2268f5767d4c52d09c1b354945737046f618fe5
f3d4adf Make p2p-acceptablock not an extended test (Matt Corallo)
00dcda6 [qa] test that invalid blocks on an invalid chain get a disconnect (Matt Corallo)
015a525 Reject headers building on invalid chains by tracking invalidity (Matt Corallo)
932f118 Accept unrequested blocks with work equal to our tip (Matt Corallo)
3d9c70c Stop always storing blocks from whitelisted peers (Matt Corallo)
3b4ac43 Rewrite p2p-acceptblock in preparation for slight behavior changes (Matt Corallo)
Pull request description:
@sdaftuar pointed out that the version in #11487 was somewhat DoS-able as someone could feed you a valid chain that forked off the the last checkpoint block and force you to do lots of work just walking backwards across blocks for each new block they gave you. We came up with a few proposals but settled on the one implemented here as likely the simplest without obvious DoS issues. It uses our existing on-load mapBlockIndex walk to make sure everything that descends from an invalid block is marked as such, and then simply caches blocks which we attempted to connect but which were found to be invalid. To avoid DoS issues during IBD, this will need to depend on #11458.
Includes tests from #11487.
Tree-SHA512: 46aff8332908e122dae72ceb5fe8cd241902c2281a87f58a5fb486bf69d46458d84a096fdcb5f3e8e07fbcf7466232b10c429f4d67855425f11b38ac0bf612e1
This tracks the set of all known invalid-themselves blocks (ie
blocks which we attempted to connect but which were found to be
invalid). This is used to cheaply check if new headers build on an
invalid chain.
While we're at it we also resolve an edge-case in invalidateblock
on pruned nodes which results in them needing a reindex if they
fail to reorg.
This is a simple change that makes our accept requirements the
same as our request requirements, (ever so slightly) further
decoupling our consensus logic from our FindNextBlocksToDownload
logic in net_processing.
A peer could try to waste our resources by sending us unrequested blocks with
low work, eg to fill up our disk. Since
e2652002b6 we no longer request blocks until we
know we're on a chain with more than nMinimumChainWork (our anti-DoS
threshold), but we would still process unrequested blocks that had more work
than our tip. This commit fixes that behavior.
258d33b41 [mempool] Mark unaccepted txs present in mempool as 'already there'. (Karl-Johan Alm)
Pull request description:
I was investigating the reasons for failed imports in mempool and noticed that `LoadMempool()` and `pwallet->postInitProcess()` (for all wallets) are executed concurrently. The wallet will end up importing transactions that `LoadMempool()` later tries to import; the latter will fail due to the tx already being in the mempool.
This PR changes the log message, adding an additional "already there" entry. For transactions not accepted into mempool, a check if they are in the mempool is done first, and if found, they are counted as 'already there', otherwise counted as 'failed'.
Also slight rewording for consistency (successes, failed, expired, ... -> succeeded, failed, expired).
Tree-SHA512: 1a6134a25260917f2768365e0dfd8b278fe3f8287cab38bb028b7de3d517718a2d37696186dc7a23ceab338cc755fbbe7d45358ee94e573610fddd2a0620d6e5
This runs Block{Connected,Disconnected}, SetBestChain, Inventory,
and TransactionAddedToMempool on the background scheduler thread.
Of those, only BlockConnected is used outside of Wallet/ZMQ, and
is used only for orphan transaction removal in net_processing,
something which does not need to be synchronous with anything
else.
This partially reverts #9583, re-enabling some of the gains from
#7946. This does not, however, re-enable the gains achieved by
repeatedly releasing cs_main between each transaction processed.
4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón)
3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón)
832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón)
3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón)
Pull request description:
...is created by individual transactions to 2 places (but call only once in each):
- ConnectBlock ( before calculated fees per txs twice )
- AcceptToMemoryPoolWorker ( before called CheckTxInputs 4 times and calculated
fees per tx one extra time )
Also call tx.GetValueOut() only once per call of CheckTxInputs (instead of 2)
For more motivation:
~~https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1493~~https://github.com/jtimon/bitcoin/compare/0.13-consensus-inputs...jtimon:0.13-consensus-inputs-comments
EDIT: partially replaces #6445
Near-Bugfix as pointed out in https://github.com/bitcoin/bitcoin/pull/8498#discussion_r124346132
Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346
c626dcb50 Make fUseCrypto atomic (MeshCollider)
731065b11 Consistent parameter names in txdb.h (MeshCollider)
35aeabec6 Make fReindex atomic to avoid race (MeshCollider)
58d91af59 Fix race for mapBlockIndex in AppInitMain (MeshCollider)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/11106
Also makes fReindex atomic as suggested in @TheBlueMatt comment below, and makes fUseCrypto atomic as suggested in 10916
d291e7635b just renames the parameters in the txdb header file to make them consistent with those used in the cpp file, noticed it when looking for uses of fReindex
Tree-SHA512: b378aa7289fd505b76565cd4d48dcdc04ac5540283ea1c80442170b0f13cb6df771b1a94dd54b7fec3478a7b4668c224ec9d795f16937782724c5d020edd3a42
6643b80 Add state message print to AcceptBlock failure message. (Matt Corallo)
Pull request description:
This should make it easier to debug issues where the CheckBlock at
the top of ProcessNewBlock fails (which does not print, in contrast
to AcceptBlock, which always prints).
This was motivated by #11371 which appears to be exactly such a case, and is not debuggable from the information provided. Not sure how much this would have helped in that case, but it is kinda weird that we can reject a block without ever printing why.
Tree-SHA512: 7a1c2c76080b810212da885c38e091609e409c62918cc326bb36a1096e09b2ae7e26fd4bdaefd79863d2894e2823e463005700a524940f177a59ef09f589b2f1
bf64c3cb3 Ignore transactions added to mempool during a reorg for fee estimation purposes. (Alex Morcos)
04f78ab5b Do not reject based on mempool min fee when bypass_limits is set. (Alex Morcos)
fd849e1b0 Change AcceptToMemoryPool function signature (Alex Morcos)
Pull request description:
First commit just removes default arguments from `AcceptToMemoryPool` and consolidates two arguments, it does not change behavior.
Second commit finally fixes the fact that we're not meant to reject based on mempool min fee when adding a transaction from a disconnected block during a reorg as mentioned [here](https://github.com/bitcoin/bitcoin/pull/9602#issue-202197849)
Third commit makes fee estimation ignore transactions added from a disconnected block during a reorg. I think this was another source of fee estimates returning estimates below 1000 sat/kB as in #11303.
Tree-SHA512: 30925ca8b341915bb214f1d2590b36b7931f2e125b7660150e38ae70338f00db5aa7f1608546dddb181446924177eb7cf62ea8bd2583068acc074d6c3f86bc0c
This should make it easier to debug issues where the CheckBlock at
the top of ProcessNewBlock fails (which does not print, in contrast
to AcceptBlock, which always prints).
3a131b724 Rename out to m_tx_out in CScriptCheck (Johnson Lau)
e91211878 [Refactor] Combine scriptPubKey and amount as CTxOut in CScriptCheck (Johnson Lau)
Pull request description:
This simplifies CScriptCheck by combining scriptPubKey and amount
Tree-SHA512: 6422363cf5394c6cfefb30c1709db6def63230b809cc7697887e4a2e8c684149208edf91dd139e031b9fe732776b2db59305f77c3cba6f333b11cceb39ef0cc2
A C-style cast is equivalent to try casting in the following order:
1. const_cast(...)
2. static_cast(...)
3. const_cast(static_cast(...))
4. reinterpret_cast(...)
5. const_cast(reinterpret_cast(...))
By using static_cast<T>(...) explicitly we avoid the possibility
of an unintentional and dangerous reinterpret_cast. Furthermore
static_cast<T>(...) allows for easier grepping of casts.
by individual transactions to 2 places (but call only once in each):
- ConnectBlock ( before calculated fees per txs twice )
- AcceptToMemoryPoolWorker ( before called CheckTxInputs 4 times and calculated
fees per tx one extra time )
Also call tx.GetValueOut() only once per call of CheckTxInputs (instead of 2)
fdc3293 Document assumptions that are being made to avoid NULL pointer dereferences (practicalswift)
Pull request description:
Document assumptions (via `assert(…)`:s) that are being made avoid `NULL` pointer dereferences.
Rationale:
* Make it clear to human reviewers and non-human static analyzers that what might look like potential `NULL` pointer dereferences are written the way they are intentionally (these cases are currently flagged by various static analyzers).
Tree-SHA512: b424328195e2680e1e4ec546298f718c49e5ad182147dc004de580693db1b50eec4065e1c4f232bdb302baa12954265a50ba21cb5ba4ff30248535b2de778672
- Move comment about transaction/block weight calculation so it applies not only to the GetBlockWeight function but also to GetTransactionWeight
- Fix comment in validation.cpp referencing future deployment of BIP113. It has already been deployed.
- The doc comment for BLOCK_DOWNLOAD_WINDOW wasn't updated since pruning was introduced, so it still refers to pruning as something that might happen in the future. A larger BLOCK_DOWNLOAD_WINDOW window would now, indeed, make pruning harder.
Combine fLimitFree and fOverrideMempoolLimit into a single boolean:
bypass_limits. This is used to indicate that mempool limiting based on feerate
should be bypassed. It is used when readding transactions from a reorg and then
the mempool is trimmed to size after all transactions are added and they can be
evaluated in the context of their descendants. No changes to behavior.
ee4d1493e Drop upgrade-cancel callback registration for a generic "resumeable" (Matt Corallo)
Pull request description:
Instead of passing a StartShutdown reference all the way up from
txdb, give ShowProgress a "cancelable" boolean, as StartShutdown
is pretty much always what you'll want to use to cancel. Use the
same boolean to allow cancel during initial block verification.
Tree-SHA512: 515817aaa4b9e3e856200e00be9c2d44ecfa2d4f288fe3e02116105fe85de2650c13076ee7e45396ec1ce6ab45e53b0477cddda7cfdee5b3bd0589cb81a4c346
c00199244 Fix potential null dereferences (MeshCollider)
Pull request description:
Picked up by the static analyzer [Facebook Infer](http://fbinfer.com/) which I was playing around with for another research project. Just adding some asserts before dereferencing potentially null pointers.
Tree-SHA512: 9c01dab2d21bce75c7c7ef867236654ab538318a1fb39f96f09cdd2382a05be1a6b2db0a1169a94168864e82ffeae0686a383db6eba799742bdd89c37ac74397
1aa97ee08 Add savemempool RPC (Lawrence Nahum)
467cbbcbf Add return value to DumpMempool (Lawrence Nahum)
Pull request description:
Adds a simple parameterless rpc command to dump the mempool.
Rationale:
Sometimes there can be a crash for whatever reason (bug, power loss, etc) causing the mempool.dat file to not be saved.
This change allows to script/cron the rpc call to have more regular saves to the file as well as cli/ad-hoc.
This should solve issue https://github.com/bitcoin/bitcoin/issues/11086
Tree-SHA512: e856ae9777425a4521279c9b58e69285d8e374790bebefd3284cf91931eac0e456f86224f427a087a01bf70440bf6e439fa02c8a34940eb1046ae473e98b6aaa
On startup, the wallets will start pumping wallet transactions into the mempool in a different thread while LoadMempool() is running.
This will sometimes result in transactions "failing" to be accepted into mempool, but only for the reason that they were already
put there by a wallet. The log message for mempool load would note this as a 'failure' to import, which was misleading; it should
instead mark it as the transaction already being in the mempool.
a473eff [bench] Replace 0.00(000)1 with MICRO/MILLI #defines in validation.cpp. (Karl-Johan Alm)
5f850b0 [bench] Include ms/blk stats in Connect* benchmarks. (Karl-Johan Alm)
Pull request description:
Display the average per block runtime for the various benchmarked times in the block connect functions to give an overview of long(er) term time distribution statistics.
Tree-SHA512: 3d6f24f6b9e3dbb448a647e2cda8e7b90ad6a16d4821f49f426a8e1ebc3ce5a0cf0a8cde82213e293affba441615702dfe50822c8c818e282af03bfe383d83e0
Instead of passing a StartShutdown reference all the way up from
txdb, give ShowProgress a "resumeable" boolean, which is used to
inform the user if the action will be resumed, but cancel is always
allowed by just calling StartShutdown().
90d4d89 scripted-diff: Use the C++11 keyword nullptr to denote the pointer literal instead of the macro NULL (practicalswift)
Pull request description:
Since C++11 the macro `NULL` may be:
* an integer literal with value zero, or
* a prvalue of type `std::nullptr_t`
By using the C++11 keyword `nullptr` we are guaranteed a prvalue of type `std::nullptr_t`.
For a more thorough discussion, see "A name for the null pointer: nullptr" (Sutter &
Stroustrup), http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2431.pdf
With this patch applied there are no `NULL` macro usages left in the repo:
```
$ git grep NULL -- "*.cpp" "*.h" | egrep -v '(/univalue/|/secp256k1/|/leveldb/|_NULL|NULLDUMMY|torcontrol.*NULL|NULL cert)' | wc -l
0
```
The road towards `nullptr` (C++11) is split into two PRs:
* `NULL` → `nullptr` is handled in PR #10483 (scripted, this PR)
* `0` → `nullptr` is handled in PR #10645 (manual)
Tree-SHA512: 3c395d66f2ad724a8e6fed74b93634de8bfc0c0eafac94e64e5194c939499fefd6e68f047de3083ad0b4eff37df9a8a3a76349aa17d55eabbd8e0412f140a297
85c82b5 Avoid masking of difficulty adjustment errors by checkpoints (Pieter Wuille)
Pull request description:
Currently difficulty adjustment violations are not reported for chains that branch off before the last checkpoint. Change this by moving the checkpoint check after the difficulty check.
Tree-SHA512: 33666f2c3459151b28c42041a463779e6df18f61d3dd5b1879a0af4e5b199ef74d1e33e06af68bebfdfb211569ad5fb56556bfebe9d63b5688d910ea211b839a
d34957e [wallet] [tests] Add keypool topup functional test (Jonas Schnelli)
095142d [wallet] keypool mark-used and topup (John Newbery)
c25d90f [wallet] Add HasUnusedKeys() helper (John Newbery)
f2123e3 [wallet] Cache keyid -> keypool id mappings (John Newbery)
83f1ec3 [wallet] Don't hold cs_LastBlockFile while calling setBestChain (John Newbery)
2376bfc [wallet] [moveonly] Move LoadKeyPool to cpp (Matt Corallo)
cab8557 [wallet] [moveonly] Move CAffectedKeysVisitor (Jonas Schnelli)
Pull request description:
This PR contains the first part of #10882 :
- if a key from the keypool is used, mark all keys up to that key as used, and then try to top up the keypool
- top up the keypool on startup
Notably, it does not stop the node or prevent the best block from advancing if the keypool drops below a threshold (which means that transactions may be missed and funds lost if restoring from an old HD wallet backup).
Tree-SHA512: ac681fefeaf7ec2aab2fa1da93d12273ea80bd05eb48d7b3b551ea6e5d975dd97ba7de52b7fba52993823280ac4079cc36cf78a27dac708107ebf8fb6326142b
Currently difficulty adjustment violations are not reported for
chains that branch off before the last checkpoint. Change this
by moving the checkpoint check after the difficulty check.
c0025d0 Fix segfault when shutting down before fully loading (Matt Corallo)
1385697 Order chainstate init more logically. (Matt Corallo)
ff3a219 Call RewindBlockIndex even if we're about to run -reindex-chainstate (Matt Corallo)
b0f3249 More user-friendly error message if UTXO DB runs ahead of block DB (Matt Corallo)
eda888e Fix some LoadChainTip-related init-order bugs. (Matt Corallo)
Pull request description:
This does a number of things to clean up chainstate init order,
fixing some issues as it goes:
* Order chainstate init more logically - first all of the
blocktree-related loading, then coinsdb, then
pcoinsTip/chainActive. Only create objects as needed.
* More clearly document exactly what is and isn't called in
-reindex and -reindex-chainstate both with comments noting
calls as no-ops and by adding if guards.
* Move the writing of fTxIndex to LoadBlockIndex - this fixes a
bug introduced in d6af06d68a where
InitBlockIndex was writing to fTxIndex which had not yet been
checked (because LoadChainTip hadn't yet initialized the
chainActive, which would otherwise have resulted in
InitBlockIndex being a NOP), allowing you to modify -txindex
without reindex, potentially corrupting your chainstate!
* Rename InitBlockIndex to LoadGenesisBlock, which is now a more
natural name for it. Also check mapBlockIndex instead of
chainActive, fixing a bug where we'd write the genesis block out
on every start.
* Move LoadGenesisBlock further down in init. This is a more logical
location for it, as it is after all of the blockindex-related
loading and checking, but before any of the UTXO-related loading
and checking.
* Give LoadChainTip a return value - allowing it to indicate that
the UTXO DB ran ahead of the block DB. This just provides a nicer
error message instead of the previous mysterious
assert(!setBlockIndexCandidates.empty()) error.
* Calls ActivateBestChain in case we just loaded the genesis
block in LoadChainTip, avoiding relying on the ActivateBestChain
in ThreadImport before continuing init process.
* Move all of the VerifyDB()-related stuff into a -reindex +
-reindex-chainstate if guard. It couldn't do anything useful
as chainActive.Tip() would be null at this point anyway.
Tree-SHA512: 3c96ee7ed44f4130bee3479a40c5cd99a619fda5e309c26d60b54feab9f6ec60fabab8cf47a049c9cf15e88999b2edb7f16cbe6819e97273560b201a89d90762
RewindBlockIndex works over both chainActive - disconnecting blocks
from the tip that need witness verification - and mapBlockIndex -
requiring redownload of blocks missing witness data.
It should never have been the case that the second half is skipped
if we're about to run -reindex-chainstate.
This gives LoadChainTip a return value - allowing it to indicate that
the UTXO DB ran ahead of the block DB. This just provides a nicer
error message instead of the previous mysterious
assert(!setBlockIndexCandidates.empty()) error.
This also calls ActivateBestChain in case we just loaded the genesis
block in LoadChainTip, avoiding relying on the ActivateBestChain
in ThreadImport before continuing init process.
* Move the writing of fTxIndex to LoadBlockIndex - this fixes a
bug introduced in d6af06d68a where
InitBlockIndex was writing to fTxIndex which had not yet been
checked (because LoadChainTip hadn't yet initialized the
chainActive, which would otherwise have resulted in
InitBlockIndex being a NOP), allowing you to modify -txindex
without reindex, potentially corrupting your chainstate!
* Rename InitBlockIndex to LoadGenesisBlock, which is now a more
natural name for it. Also check mapBlockIndex instead of
chainActive, fixing a bug where we'd write the genesis block out
on every start.
095b917 Avoid using sizes on non-fixed-width types to derive protocol constants. (Gregory Maxwell)
Pull request description:
Thanks to awemany for pointing this out.
This replaces #10172 which appears to be abandoned, but uses the constants as requested on that PR.
Tree-SHA512: 032c0d75b3aaf807a7d0c7fb8ff5515acc45ad58bd00fe81413f900fe02bad900534a970403b9bb568e132c9eddea6043e958daf625e8acc84375bd41ee2e2ef
d9d1bd3 nCheckDepth chain height fix (romanornr)
Pull request description:
````
if (nCheckDepth <= 0)
nCheckDepth = 1000000000; // suffices until the year 19000
if (nCheckDepth > chainActive.Height())
nCheckDepth = chainActive.Height();
````
These lines confuse me.
Correct me if I am wrong, but we can't check any more blocks than we have right?
If someone requests <= 0 it get set it into some huge number and then immediately limit it to the chain height in the following statement.
````
if (nCheckDepth > chainActive.Height())
nCheckDepth = chainActive.Height();
````
when using ````--checkblocks=Z```` When Z is ````0```` or any other negative number, it will check all blocks.
I think it should be changed to this maybe.
````
if (nCheckDepth <= 0 || nCheckDepth > chainActive.Height())
nCheckDepth = chainActive.Height();
````
Which gets rid of that huge number which is confusing for any other altcoins that have a different block time.
Tree-SHA512: 8ee0ae5f33b399fa74dc16926709694ccfe1fc8a043cba2f5d00884220ac1b9b13f2df4588041f4133be634e5c7b14f4eebe24294028dafe91581a97dbe627f3
Some people keep thinking that MAX_BLOCK_BASE_SIZE is a separate
size limit from the weight limit when it fact it is superfluous,
and used in early tests before the witness data has been
validated or just to compute worst case sizes. The size checks
that use it would not behave any differently consensus wise
if they were eliminated completely.
Its correct value is not independently settable but is a function
of the weight limit and weight formula.
This patch just eliminates it and uses the scale factor as
required to compute the worse case constants.
It also moves the weight factor out of primitives into consensus,
which is a more logical place for it.
18bacec6c Make check to distinguish between orphan txs and old txs more efficient. (Alex Morcos)
Tree-SHA512: b6b4bad89aa561975dce7b68b2fdad5623af5ebcb9c38fd6a72b5f6d0544ed441df4865591ac018f7ae0df9b5c60820cb4d9e55664f5667c9268458df70fd554
69a4339ef missing white space in function arg (Lawrence Nahum)
Pull request description:
Tree-SHA512: 94d832eca0b5429cf48c7c1d4489942b53182c5b7ad9f24264867e7631301173c23a48f7b2e8fd22ff1b3137bafacb3f9d4320f4df235668c9096aaefff7afe8
176c021 [qa] Test non-atomic chainstate writes (Suhas Daftuar)
d6af06d Dont create pcoinsTip until after ReplayBlocks. (Matt Corallo)
eaca1b7 Random db flush crash simulator (Pieter Wuille)
0580ee0 Adapt memory usage estimation for flushing (Pieter Wuille)
013a56a Non-atomic flushing using the blockchain as replay journal (Pieter Wuille)
b3a279c [MOVEONLY] Move LastCommonAncestor to chain (Pieter Wuille)
Tree-SHA512: 47ccc62303f9075c44d2a914be75bd6969ff881a857a2ff1227f05ec7def6f4c71c46680c5a28cb150c814999526797dc05cf2701fde1369c06169f46eccddee
Check that cached script execution results are only valid for the same
script flags; that script execution checks are returned for non-cached
transactions; and that cached results are only valid for transactions
with the same witness hash.
Checking for the existence in the CCoinsViewCache of the outputs of a new tx
will result in a disk hit for every output since they will not be found. On the
other hand if those outputs exist already, then the inputs must also have been
missing, so we can move this check inside the input existence check so in the
common case of a new tx it doesn't need to run.
The purpose of the check is to avoid spamming the orphanMap with slightly old
txs which we have already seen in a block, but it is already only optimistic
(depending on the outputs not being spent), so make it even more efficient by
only checking the cache and not the entire pcoinsTip.
This requires that we not access pcoinsTip in InitBlockIndex's
FlushStateToDisk (so we just skip it until later in AppInitMain)
and the LoadChainTip in LoadBlockIndex (which there is already one
later in AppinitMain, after ReplayBlocks, so skipping it there is
fine).
Includes some simplifications by Suhas Daftuar and Pieter Wuille.
5a9b508 [trivial] Add end of namespace comments (practicalswift)
Tree-SHA512: 92b0fcae4d1d3f4da9e97569ae84ef2d6e09625a5815cd0e5f0eb6dd2ecba9852fa85c184c5ae9de5117050330ce995e9867b451fa8cd5512169025990541a2b
This wraps CheckInputs in ATMP's cache-inputs call to check that
each scriptPubKey the CCoinsViewCache provides is the one which
was committed to by the input's transaction hash.
A few "a->an" and "an->a".
"Shows, if the supplied default SOCKS5 proxy" -> "Shows if the supplied default SOCKS5 proxy". Change made on 3 occurrences.
"without fully understanding the ramification of a command" -> "without fully understanding the ramifications of a command".
Removed duplicate words such as "the the".
9417d7a33 Be much more agressive in AccessCoin docs. (Matt Corallo)
f58349ca8 Restore some assert semantics in sigop cost calculations (Matt Corallo)
3533fb4d3 Return a bool in SpendCoin to restore pre-per-utxo assert semantics (Matt Corallo)
ec1271f2b Remove useless mapNextTx lookup in CTxMemPool::TrimToSize. (Matt Corallo)
Tree-SHA512: 158a4bce063eac93e1d50709500a10a7cb1fb3271f10ed445d701852fce713e2bf0da3456088e530ab005f194ef4a2adf0c7cb23226b160cecb37a79561f29ca
d9bec888f Use REJECT_DUPLICATE for already known and conflicted txn (Pieter Wuille)
Tree-SHA512: adc6dc5caed731c3fd5c8784e8820a074e320360cdb5579c5b9299f9799dd99de60b7382d336ab1909dab8b23e744456d78aa0c3b1c8dd1af3d1b779314cf8fa
3ff1fa8 Use override keyword on CCoinsView overrides (Russell Yanofsky)
24e44c3 Don't return stale data from CCoinsViewCache::Cursor() (Russell Yanofsky)
Tree-SHA512: 08699dae0925ffb9c018f02612ac6b7eaf73ec331e2f4f934f1fe25a2ce120735fa38596926e924897c203f7470e99f0a99cf70d2ce31ff428b105e16583a861
CCoinsViewCache doesn't actually support cursor iteration returning the
current contents of the cache, so raise an error when the cursor method is
called instead of returning a cursor that iterates over stale data.
Also update the gettxoutsetinfo RPC which was relying on the old behavior to be
explicit about which view it is returning data about.
This adds a new CuckooCache in validation, caching whether all of a
transaction's scripts were valid with a given set of script flags.
Unlike previous attempts at caching an entire transaction's
validity, which have nearly universally introduced consensus
failures, this only caches the validity of a transaction's
scriptSigs. As these are pure functions of the transaction and
data it commits to, this should be much safer.
This is somewhat duplicative with the sigcache, as entries in the
new cache will also have several entries in the sigcache. However,
the sigcache is kept both as ATMP relies on it and because it
prevents malleability-based DoS attacks on the new higher-level
cache. Instead, the -sigcachesize option is re-used - cutting the
sigcache size in half and using the newly freed memory for the
script execution cache.
Transactions which match the script execution cache never even have
entries in the script check thread's workqueue created.
Note that the cache is indexed only on the script execution flags
and the transaction's witness hash. While this is sufficient to
make the CScriptCheck() calls pure functions, this introduces
dependancies on the mempool calculating things such as the
PrecomputedTransactionData object, filling the CCoinsViewCache, etc
in the exact same way as ConnectBlock. I belive this is a reasonable
assumption, but should be noted carefully.
In a rather naive benchmark (reindex-chainstate up to block 284k
with cuckoocache always returning true for contains(),
-assumevalid=0 and a very large dbcache), this connected blocks
~1.7x faster.
589827975 scripted-diff: various renames for per-utxo consistency (Pieter Wuille)
a5e02bc7f Increase travis unit test timeout (Pieter Wuille)
73de2c1ff Rename CCoinsCacheEntry::coins to coin (Pieter Wuille)
119e552f7 Merge CCoinsViewCache's GetOutputFor and AccessCoin (Pieter Wuille)
580b02309 [MOVEONLY] Move old CCoins class to txdb.cpp (Pieter Wuille)
8b25d2c0c Upgrade from per-tx database to per-txout (Pieter Wuille)
b2af357f3 Reduce reserved memory space for flushing (Pieter Wuille)
41aa5b79a Pack Coin more tightly (Pieter Wuille)
97072d668 Remove unused CCoins methods (Pieter Wuille)
ce23efaa5 Extend coins_tests (Pieter Wuille)
508307968 Switch CCoinsView and chainstate db from per-txid to per-txout (Pieter Wuille)
4ec0d9e79 Refactor GetUTXOStats in preparation for per-COutPoint iteration (Pieter Wuille)
13870b56f Replace CCoins-based CTxMemPool::pruneSpent with isSpent (Pieter Wuille)
05293f3cb Remove ModifyCoins/ModifyNewCoins (Pieter Wuille)
961e48397 Switch tests from ModifyCoins to AddCoin/SpendCoin (Pieter Wuille)
8b3868c1b Switch CScriptCheck to use Coin instead of CCoins (Pieter Wuille)
c87b957a3 Only pass things committed to by tx's witness hash to CScriptCheck (Matt Corallo)
f68cdfe92 Switch from per-tx to per-txout CCoinsViewCache methods in some places (Pieter Wuille)
000391132 Introduce new per-txout CCoinsViewCache functions (Pieter Wuille)
bd83111a0 Optimization: Coin&& to ApplyTxInUndo (Pieter Wuille)
cb2c7fdac Replace CTxInUndo with Coin (Pieter Wuille)
422634e2f Introduce Coin, a single unspent output (Pieter Wuille)
7d991b55d Store/allow tx metadata in all undo records (Pieter Wuille)
c3aa0c119 Report on-disk size in gettxoutsetinfo (Pieter Wuille)
d34242430 Remove/ignore tx version in utxo and undo (Pieter Wuille)
7e0032290 Add specialization of SipHash for 256 + 32 bit data (Pieter Wuille)
e484652fc Introduce CHashVerifier to hash read data (Pieter Wuille)
f54580e7e error() in disconnect for disk corruption, not inconsistency (Pieter Wuille)
e66dbde6d Add SizeEstimate to CDBBatch (Pieter Wuille)
Tree-SHA512: ce1fb1e40c77d38915cd02189fab7a8b125c7f44d425c85579d872c3bede3a437760997907c99d7b3017ced1c2de54b2ac7223d99d83a6658fe5ef61edef1de3
As the maximum amount of data that can be pulled into the cache due to
a block validation is much lower now (at most one CCoin entry per input
and per output), reduce the conservative estimate used to determine
flushing time.
This patch makes several related changes:
* Changes the CCoinsView virtual methods (GetCoins, HaveCoins, ...)
to be COutPoint/Coin-based rather than txid/CCoins-based.
* Changes the chainstate db to a new incompatible format that is also
COutPoint/Coin based.
* Implements reconstruction code for hash_serialized_2.
* Adapts the coins_tests unit tests (thanks to Russell Yanofsky).
A side effect of the new CCoinsView model is that we can no longer
use the (unreliable) test for transaction outputs in the UTXO set
to determine whether we already have a particular transaction.
This clarifies a bit more the ways in which the new script execution
cache could break consensus in the future if additional data from
the CCoins object were to be used as a part of script execution.
After this change, any such consensus breaks should be very visible
to reviewers, hopefully ensuring no such changes can be made.
The earlier CTxInUndo class now holds the same information as the Coin
class. Instead of duplicating functionality, replace CTxInUndo with a
serialization adapter for Coin.
Previously, transaction metadata (height, coinbase or not, and before
the previous commit also nVersion) was only stored for undo records
that correspond to the last output of a transaction being spent.
This only saves 2 bytes per undo record. Change this to storing this
information for every undo record, and stop complaining for having it
in non-last output spends. This means that undo dat written with
this patch won't be readable by older versions anymore.
This makes the following changes:
* In undo data and the chainstate database, the transaction nVersion
field is removed from the data structures, always written as 0, and
ignored when reading.
* The definition of hash_serialized in gettxoutsetinfo is changed to no
longer incude the nVersion field. It is renamed to hash_serialized_2
to avoid confusion. The new definition also includes transaction
height and coinbase information, as this information was missing
before.
This depends on having a CHashVerifier-based undo data checksum
verifier.
Apart from changing the definition of serialized_hash, downgrading
after using this patch is supported, as no release ever used the value
of nVersion field in UTXO entries.
This is necessary later, when we drop the nVersion field from the undo
data. At that point deserializing and reserializing the data won't
roundtrip anymore, and thus that approach can't be used to verify
checksums anymore.
With this CHashVerifier approach, we can deserialize while hashing the
exact serialized form that was used. This is both more efficient and
more correct in that case.
The error() function unconditionally reports an error. It should only
be used for actually exception situations, and not for the type of
inconsistencies that ApplyTxInUndo/DisconnectBlock can graciously deal
with.
This also makes a subtle semantics change: in ApplyTxInUndo, when a
record with metadata is encountered (indicating it is the last spend
from a tx), don't wipe the CCoins record if it wasn't empty at that
point. This makes sure that UTXO operations never affect any other
UTXOs (including those from the same tx).
211adc0 Use range-based for loops (C++11) when looping over vector elements (practicalswift)
Tree-SHA512: 0e007f20dcef99d3c7a1036265e00f689d69f42e02fd82dd8389f45b52d31947e5f9388de2610d3d9bd9f554915ce0d35ebce561e5ae3a9013956d0ee4937145
Rather than re-add disconnected block transactions back to the mempool
immediately, store them in a separate disconnectpool for later processing,
because we expect most such transactions to reappear in the chain that is
still to be connected (and thus we can avoid the work of reprocessing those
transactions through the mempool altogether).
DisconnectBlock currently has a complicated interface:
Situation Return value
pfClean != nullptr pfClean == nullptr
All good: true true
Failure: false false
Unclean rewind: true false
with *pfClean=false
Change this to return a tristate enum instead. As an added bonus,
remove the ValidationState& argument which was unused.
68af651 MOVEONLY: move TxConfirmStats to cpp (Alex Morcos)
2332f19 Initialize TxConfirmStats in constructor (Alex Morcos)
5ba81e5 Read and Write fee estimate file directly from CBlockPolicyEstimator (Alex Morcos)
14e10aa Call estimate(Smart)Fee directly from CBlockPolicyEstimator (Alex Morcos)
dbb9e36 Give CBlockPolicyEstimator it's own lock (Alex Morcos)
f6187d6 Make processBlockTx private. (Alex Morcos)
ae7327b Make feeEstimator its own global instance of CBlockPolicyEstimator (Alex Morcos)
Tree-SHA512: dbf3bd2b30822e609a35f3da519b62d23f8a50e564750695ddebd08553b4c01874ae3e07d792c6cc78cc377d2db33b951ffedc46ac7edaf5793f9ebb931713af
This removes another callback from block connection logic, making it
easier to reason about the wallet-RPCs-returns-stale-info issue.
UpdatedTransaction was previously used by the GUI to display
coinbase transactions only after they have a block built on top of
them. This worked fine for in most cases, but only worked due to a
corner case if the user received a coinbase payout in a block
immediately prior to restart. In that case, the normal process of
caching the most recent coinbase transaction's hash would not work,
and instead it would only work because of the on-load -checkblocks
calling DisconnectBlock and ConnectBlock on the current tip.
In order to make this more robust, a full mapWallet loop after the
first block which is connected after restart was added.
b1a6d4c Take a CTransactionRef in AddToWalletIfInvolvingMe to avoid a copy (Matt Corallo)
1c95e2f Use std::shared_ptr instead of boost::shared_ptr in ScriptForMining (Matt Corallo)
91f1e6c Remove dead-code tracking of requests for blocks we generated (Matt Corallo)
acad82f Add override to functions using CValidationInterface methods (Matt Corallo)
e6d5e6c Hold cs_wallet for whole block [dis]connection processing (Matt Corallo)
461e49f SyncTransaction->TxAddedToMempool/BlockConnected/Disconnected (Matt Corallo)
f404334 Handle SyncTransaction in ActivateBestChain instead of ConnectTrace (Matt Corallo)
a147687 Keep conflictedTxs in ConnectTrace per-block (Matt Corallo)
d3167ba Handle conflicted transactions directly in ConnectTrace (Matt Corallo)
29e6e23 Make ConnectTrace::blocksConnected private, hide behind accessors (Matt Corallo)
822000c Add pblock to connectTrace at the end of ConnectTip, not start (Matt Corallo)
f5e9a01 Include missing #include in zmqnotificationinterface.h (Matt Corallo)
Tree-SHA512: 8893d47559da3b28d2ef7359768547cba8a4b43b6f891d80f5848f995a84b1517bfb0f706fdc8cd43f09a1350349eb440d9724a59363ab517dfcc4fcb31b2018
This simplifies fixing the wallet-returns-stale-info issue as we
can now hold cs_wallet across an entire block instead of only
per-tx (though we only actually do so in the next commit).
This change also removes the NOT_IN_BLOCK constant in favor of only
passing the CBlockIndex* parameter to SyncTransactions when a new
block is being connected, instead of also when a block is being
disconnected.
This change adds a parameter to BlockConnectedDisconnected which
lists the transactions which were removed from mempool due to
confliction as a result of this operation. While its somewhat of a
shame to make block-validation-logic generate a list of mempool
changes to be included in its generated callbacks, fixing this isnt
too hard.
Further in this change-set, CValidationInterface starts listening
to mempool directly, placing it in the middle and giving it a bit
of logic to know how to route notifications from block-validation,
mempool, etc (though not listening for conflicted-removals yet).
f110272 Remove `namespace fs=fs` (Wladimir J. van der Laan)
75594bd torcontrol: Use fs::path instead of std::string for private key path (Wladimir J. van der Laan)
2a5f574 Use fsbridge for fopen and freopen (Wladimir J. van der Laan)
bac5c9c Replace uses of boost::filesystem with fs (Wladimir J. van der Laan)
7d5172d Replace includes of boost/filesystem.h with fs.h (Wladimir J. van der Laan)
19e36bb Add fs.cpp/h (Wladimir J. van der Laan)
Tree-SHA512: 2c34f059dfa6850b9323f3389e9090a6b5f839a457a2960d182c2ecfafd9883c956f5928bb796613402d3aad68ebc78259796a7a313f4a6cfa98aaf507a66842
This changes the logging categories to boolean flags instead of strings.
This simplifies the acceptance testing by avoiding accessing a scoped
static thread local pointer to a thread local set of strings. It
eliminates the only use of boost::thread_specific_ptr outside of
lockorder debugging.
This change allows log entries to be directed to multiple categories
and makes it easy to change the logging flags at runtime (e.g. via
an RPC, though that isn't done by this commit.)
It also eliminates the fDebug global.
Configuration of unknown logging categories now produces a warning.
Since we are more accurately measuring pcoinsTip peak usage at twice the current in dynamic usage, it makes sense to double the default (this will lead to the same effective usage and peak usage as previously).
We should also double the buffer used to avoid flushing if above 90% but still sufficient space remaining.
4d51e9b Assert ConnectBlock block and pIndex are the same block (NicolasDorier)
972714c pow: GetNextWorkRequired never called with NULL pindexLast (Daniel Cousens)
cc44c8f ContextualCheckBlockHeader should never have pindexPrev to NULL (NicolasDorier)
Tree-SHA512: 7cc568bf9417267c335f21ec3d1505b26e56e5b3d5f4d3dbb555279489800aaa65a3bcd7bc376e274dd102912aec16ddbb18de2e2060b2667b41eb979cd9321e
This a breaking API change to the prioritisetransaction RPC call which previously required exactly three arguments and now requires exactly two (hash and feeDelta). The function prioritiseTransaction is also updated.
Remove -limitfreerelay and always enforce minRelayTxFee in the mempool (except from disconnected blocks)
Remove -relaypriority, the option was only used for the ability to allow free transactions to be relayed regardless of their priority. Both notions no longer apply.
