fa6b053b5c mempool: persist with XOR (MarcoFalke)
Pull request description:
Currently the `mempool.dat` file stores data received from remote peers as-is. This may be problematic when a program other than Bitcoin Core tries to interpret them by accident. For example, an anti-virus program or other program may scan the file and move it into quarantine, or delete it, or corrupt it.
While the local wallet is expected to re-submit any pending transactions, unrelated transactions may be missing from the mempool after a restart. This may cause fee estimates to be off, or may cause block relay to be slower.
Fix this, similar to https://github.com/bitcoin/bitcoin/pull/6650, by rolling a random XOR pattern over the dat file when writing or reading it.
Obviously this can only protect against programs that accidentally and unintentionally are trying to mess with the dat file. Any program that intentionally wants to mess with the dat file can still trivially do so.
ACKs for top commit:
achow101:
re-ACK fa6b053b5c
glozow:
reACK fa6b053b5c
ismaelsadeeq:
ACK fa6b053b5c
Tree-SHA512: ded2ce3d81bc944b828263534e3178a1e45a914fe8e024f4a14c6561a73e301820944ecc75dd704b3d4221a7a3a5c0597ccab79546250c1197609ee981fe324e
3b70f7b615 doc: fix broken doc/design/multiprocess.md links after #24352 (Ryan Ofsky)
6d43aad742 span: Make Span template deduction guides work in SFINAE context (Ryan Ofsky)
8062c3bdb9 util: Add ArgsManager SetConfigFilePath method (Ryan Ofsky)
441d00c60f interfaces: Rename CalculateBumpFees methods to be compatible with capn'proto (Ryan Ofsky)
156f49d682 interfaces: Change getUnspentOutput return type to avoid multiprocess segfault (Ryan Ofsky)
4978754c00 interfaces: Add schedulerMockForward method so mockscheduler RPC can work across processes (Ryan Ofsky)
924327eaf3 interfaces: Fix const virtual method that breaks multiprocess support (Ryan Ofsky)
82a379eca8 streams: Add SpanReader ignore method (Russell Yanofsky)
Pull request description:
This is a collection of small changes to interfaces and code which were needed as part of multiprocess PR #10102, but have been moved here to make that PR smaller.
All of these changes are refactoring changes which do not affect behavior of current code
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 3b70f7b615
naumenkogs:
ACK 3b70f7b615
maflcko:
re-ACK 3b70f7b615🎆
Tree-SHA512: 2368772b887056ad8a9f84c299cfde76ba45943770e3b5353130580900afa9611302195b899ced7b6e303b11f053ff204cae7c28ff4e12c55562fcc81119ba4c
4dd94ca18f [refactor] remove access to mapTx in validation_block_tests (TheCharlatan)
d0cd2e804e [refactor] rewrite BlockAssembler inBlock and failedTx as sets of txids (glozow)
55b0939cab scripted-diff: rename vTxHashes to txns_randomized (TheCharlatan)
a03aef9cec [refactor] rewrite vTxHashes as a vector of CTransactionRef (glozow)
938643c3b2 [refactor] remove access to mapTx in validation.cpp (glozow)
333367a940 [txmempool] make CTxMemPoolEntry::lockPoints mutable (glozow)
1bf4855016 [refactor] use CheckPackageLimits for checkChainLimits (glozow)
dbc5bdbf59 [refactor] remove access to mapTx.find in mempool_tests.cpp (glozow)
f80909e7a3 [refactor] remove access to mapTx in blockencodings_tests.cpp (glozow)
8892d6b744 [refactor] remove access to mapTx from rpc/mempool.cpp (glozow)
fad61aa561 [refactor] get wtxid from entry instead of vTxHashes (glozow)
9cd8cafb77 [refactor] use exists() instead of mapTx.find() (glozow)
14804699e5 [refactor] remove access to mapTx from policy/rbf.cpp (glozow)
1c6a73abbd [refactor] Add helper for retrieving mempool entry (TheCharlatan)
453b4813eb [refactor] Add helper for iterating through mempool entries (stickies-v)
Pull request description:
Motivation
* It seems preferable to use stdlib data structures instead of boost if they can achieve close to the same thing.
* Code external to mempool should ideally use its public helper methods instead of accessing `mapTx` or its iterators directly.
* Reduce the number of complex boost multi index type interactions
* Also see #28335 for further context/motivation. This PR together with #28385 simplifies that one.
Overview of things done in this PR:
* Make `vTxHashes` a vector of transaction references instead of a pair of transaction hash and iterator. The trade off here is that the data is retrieved on the fly with `GetEntry` instead of being cached in `vTxHashes`.
* Introduce `GetEntry` helper method to replace the more involved `GetIter` where applicable
* Replace `mapTx` access with `CTxMemPool` helper methods
* Simplify `checkChainLimits` call in `node/interfaces.cpp`
* Make `CTxMemPoolEntry`s `lockPoints`mutable such that they can be changed with a const iterator directly instead of going through `mapTx`
* Make `BlockAssembler`'s `inBlock` and `failedTx` sets of transaction hashes.
ACKs for top commit:
glozow:
reACK 4dd94ca
maflcko:
re-ACK 4dd94ca18f👝
stickies-v:
re-ACK 4dd94ca18f
Tree-SHA512: c4d043f2186e4fde337591883fac66cade3058173987b49502bd65cecf69207a3df1077f6626809652ab63230013167b7f39a2b39f1c5166959e5495df57065f
The behavior is the same as CalculateMemPoolAncestors. The only
difference is the string returned, and the string is discarded anyway
since checkChainLimits only cares about pass/fail.
Sometimes we are just interested in the order in which transactions
would be included in a block (we want to "linearize" the transactions).
Track and store this information.
This doesn't change any of the bump fee calculations.
Add an option to keep building the template regardless of feerate. We
can't just use target_feerate=0 because it's possible for transactions
to have negative modified feerates.
No behavior change for users that pass in a target_feerate.
This is primarily intended for linearizing a package of transactions
prior to submitting them to mempool. Note that, if this ctor is used,
bump fees will not be calculated because we haven't instructed MiniMiner
which outpoints for which we want bump fees to be calculated.
No behavior change. All we are doing is copying out these values before
passing them into the ctor instead of within the ctor.
This makes it possible to use the MiniMiner algorithms to analyze
transactions that haven't been submitted to the mempool yet.
It also iwyu's the mini_miner includes.
Coin serialize method segfaults if IsSpent condition is true. This caused
multiprocess code to segfault when serializing the Coin& output argument to of
the Node::getUnspentOutput method if the coin was not found. Segfault could be
triggered by double clicking and viewing transaction details in the GUI
transaction list.
Fix this by replacing Coin& output argument with optional<Coin> return value to
avoid trying to serializing spent coins.
fa05a726c2 tidy: modernize-use-emplace (MarcoFalke)
Pull request description:
Constructing a temporary unnamed object only to copy or move it into a container seems both verbose in code and a strict performance penalty.
Fix both issues via the `modernize-use-emplace` tidy check.
ACKs for top commit:
Sjors:
re-utACK fa05a726c2
hebasto:
ACK fa05a726c2.
TheCharlatan:
ACK fa05a726c2
Tree-SHA512: 4408a094f406e7bf6c1468c2b0798f68f4d952a1253cf5b20bdc648ad7eea4a2c070051fed46d66fd37bce2ce6f85962484a1d32826b7ab8c9baba431eaa2765
d27b9a2248 test: fix feature_init.py file perturbation (Martin Zumsande)
ad66ca1e47 init: abort loading of blockindex in case of missing height. (Martin Zumsande)
Pull request description:
When the block index database is non-contiguous due to file corruption (i.e. it contains indexes of height `x-1` and `x+1`, but not `x`), bitcoind can currently crash with an assert in `BuildSkip()` / `GetAncestor()` during `BlockManager::LoadBlockIndex()`:
```
bitcoind: chain.cpp:112: const CBlockIndex* CBlockIndex::GetAncestor(int) const: Assertion `pindexWalk->pprev' failed.
```
This PR changes it such that we instead return an `InitError` to the user.
I stumbled upon this because I noticed that the file perturbation in `feature_init.py` wasn't working as intended, which is fixed in the second commit:
* Opening the file twice in one `with` statement would lead to `tf_read` being empty, so the test wouldn't perturb anything but replace the file with a new one. Fixed by first opening for read, then for write.
* We need to restore the previous state after perturbations, so that only the current perturbation is active and not a mix of the current and previous ones.
* I also added `checkblocks=200` to the startup parameters so that corruption in earlier blocks of `blk00000.dat` is detected during init verification and not ignored.
After fixing `feature_init.py` like that I'd run into the `assert` mentioned above (so running the testfix from the second commit without the first one is a way to reproduce it).
ACKs for top commit:
achow101:
ACK d27b9a2248
furszy:
Code ACK d27b9a224
fjahr:
Code review ACK d27b9a2248
Tree-SHA512: 2e54da6030c5813c86bd58f816401e090bb43c5b834764a5e3c0e55dbfe09e423f88042cab823db3742088204b274d4ad2abf58a3832a4b18328b11a30bf7094
When using an assumedvalid (snapshot) chainstate along with a background
chainstate, we are syncing two very different regions of the chain
simultaneously. If we use the same blockfile space for both of these
syncs, wildly different height blocks will be stored alongside one
another, making pruning ineffective.
This change implements a separate blockfile cursor for the assumedvalid
chainstate when one is in use.
Use the expected AssumeutxoData in order to bootstrap nChainTx values
for assumedvalid blockindex entries in the snapshot chainstate. This
is necessary because nChainTx is normally built up from nTx values,
which are populated using blockdata which the snapshot chainstate
does not yet have.
Introduces ChainstateManager::GetPruneRange().
The prune budget is split evenly between the number of chainstates,
however the prune budget may be exceeded if the resulting shares are
beneath `MIN_DISK_SPACE_FOR_BLOCK_FILES`.
d8041d4e04 blockstorage: Return on fatal undo file flush error (TheCharlatan)
f0207e0030 blockstorage: Return on fatal block file flush error (TheCharlatan)
5671c15f45 blockstorage: Mark FindBlockPos as nodiscard (TheCharlatan)
Pull request description:
The goal of this PR is to establish that fatal blockstorage flush errors should be treated as errors at their call site.
Prior to this patch `FlushBlockFile` may have failed without returning in `Chainstate::FlushStateToDisk`, leading to a potential write from `WriteBlockIndexDB` that may refer to a block that is not fully flushed to disk yet. By returning if either `FlushUndoFile` or `FlushBlockFile` fail, we ensure that no further write operations take place that may lead to an inconsistent database when crashing. Add `[[nodiscard]]` annotations to them such that they are not ignored in future.
Functions that call either `FlushUndoFile` or `FlushBlockFile`, need to handle these extra abort cases properly. Since `Chainstate::FlushStateToDisk` already produces an abort error in case of `WriteBlockIndexDB` failing, no extra logic for functions calling `Chainstate::FlushStateToDisk` is required.
Besides `Chainstate::FlushStateToDisk`, `FlushBlockFile` is also called by `FindBlockPos`, while `FlushUndoFile` is only called by `FlushBlockFile` and `WriteUndoDataForBlock`. For both these cases, the flush error is not further bubbled up. Instead, the error is logged and a comment is provided why bubbling up an error would be less desirable in these cases.
---
This pull request is part of a larger effort towards improving the shutdown / abort / fatal error handling in validation code. It is a first step towards implementing proper fatal error return type enforcement similar as proposed by theuni in this pull request [comment](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1563561502). For ease of review of these critical changes, a first step would be checking that `AbortNode` leads to early and error-conveying returns at its call site. Further work for enforcing returns when `AbortNode` is called is done in https://github.com/bitcoin/bitcoin/pull/27862.
ACKs for top commit:
stickies-v:
re-ACK d8041d4
ryanofsky:
Code review ACK d8041d4e04
Tree-SHA512: 47ade9b873b15e567c8f60ca538d5a0daf32163e1031be3212a3a45eb492b866664b225f2787c9e40f3e0c089140157d8fd1039abc00c7bdfeec1b52ecd7e219
This was only explicitly used in the tests, where it can be replaced by
wrapping the original raw file pointer into a CAutoFile on creation and
then calling CAutoFile::fclose().
Also, it was used in LoadExternalBlockFile(), where it can also be
replaced by the (implicit call to the) CAutoFile destructor after
wrapping the original raw file pointer in a CAutoFile.
f18f9ef4d3 Amend bumpfee for inputs with overlapping ancestry (Murch)
2e35e944da Bump unconfirmed parent txs to target feerate (Murch)
3e3e052411 coinselection: Move GetSelectionWaste into SelectionResult (Andrew Chow)
c57889da66 [node] interface to get bump fees (glozow)
c24851be94 Make MiniMinerMempoolEntry fields private (Murch)
ac6030e4d8 Remove unused imports (Murch)
d2f90c31ef Fix calculation of ancestor set feerates in test (Murch)
a1f7d986e0 Match tx names to index in miniminer overlap test (Murch)
Pull request description:
Includes some commits to address follow-ups from #27021: https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1554675156
Reduces the effective value of unconfirmed UTXOs by the fees necessary to bump their ancestor transactions to the same feerate.
While the individual UTXOs always account for their full ancestry before coin-selection, we can correct potential overestimates with a second pass where we establish the ancestry and bump fee for the whole input set collectively.
Fixes#9645Fixes#9864Fixes#15553
ACKs for top commit:
S3RK:
ACK f18f9ef4d3
ismaelsadeeq:
ACK f18f9ef4d3
achow101:
ACK f18f9ef4d3
brunoerg:
crACK f18f9ef4d3
t-bast:
ACK f18f9ef4d3, I reviewed the latest changes and run e2e tests against eclair, everything looks good 👍
Tree-SHA512: b65180c4243b1f9d13c311ada7a1c9f2f055d530d6c533b78c2068b50b8c29ac1321e89e85675b15515760d4f1b653ebd9da77b37c7be52d9bc565a3538f0aa6
d506765199 [refactor] Remove compat.h from kernel headers (TheCharlatan)
36193af47c [refactor] Remove netaddress.h from kernel headers (TheCharlatan)
2b08c55f01 [refactor] Add CChainParams member to CConnman (TheCharlatan)
f0d1d8b35c [refactor] Add missing includes for next commit (TheCharlatan)
534b314a74 kernel: Move MessageStartChars to its own file (TheCharlatan)
9be330b654 [refactor] Define MessageStartChars as std::array (TheCharlatan)
37e2b01113 [refactor] Allow std::array<std::byte, N> in serialize.h (MarcoFalke)
Pull request description:
This removes the non-consensus critical `protocol.h` and `netaddress.h` headers from the kernel headers. With this patch, they are no longer required to include in order to use the libbitcoinkernel library. This also allows for the removal of the `compat.h` header from the kernel headers.
As an added future benefit it also reduces the number of of kernel headers that include the platform specific `bitcoin-config.h`.
For those interested, the currently required kernel headers can be inspected visually with the [sourcetrail](https://github.com/CoatiSoftware/Sourcetrail) tool by looking at the required includes of `bitcoin-chainstate.cpp`.
---
This is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587), namely its stage 1 step 3: Decouple most non-consensus headers from libbitcoinkernel.
ACKs for top commit:
stickies-v:
re-ACK d506765
hebasto:
ACK d506765199.
ajtowns:
utACK d506765199
MarcoFalke:
lgtm ACK d506765199🍛
Tree-SHA512: 6f90ea510a302c2927e84d16900e89997c39b8ff3ce9d4effeb8a134bd29cc52bd9e81e51aaa11f7496bad00025b78a58b88c5a9e0bb3f4ebbe9a76309215fb7
Follow-up from #27021: accessing of fields in MiniMinerMempoolEntry was
done inconsistently. Even though we had a getter, we would directly
write to the fields when we needed to update them.
This commits sets the fields to private and introduces a method for
updating the ancestor information in transactions using the same method
name as used for Mempool Entries.
The protocol.h file contains many non-consensus related definitions and
should thus not be part of the libbitcoinkernel. This commit makes
protocol.h no longer a required include for users of the
libbitcoinkernel.
This commit is part of the libbitcoinkernel project, namely its stage 1
step 3: Decouple most non-consensus headers from libbitcoinkernel.
Co-Authored-By: Cory Fields <cory-nospam-@coryfields.com>
fae405556d scripted-diff: Rename CBlockTreeDB -> BlockTreeDB (MarcoFalke)
faf63039cc Fixup style of moved code (MarcoFalke)
fa65111b99 move-only: Move CBlockTreeDB to node/blockstorage (MarcoFalke)
fa8685597e index: Drop legacy -txindex check (MarcoFalke)
fa69148a0a scripted-diff: Use blocks_path where possible (MarcoFalke)
Pull request description:
The only reason for the check was to print a warning about an increase in storage use. Now that 22.x is EOL and everyone should have migrated (or decided to not care about storage use), remove the check.
Also, a move-only commit is included. (Rebased from https://github.com/bitcoin/bitcoin/pull/22242)
ACKs for top commit:
TheCharlatan:
ACK fae405556d, though I lack historical context to really judge the second commit fa8685597e.
stickies-v:
ACK fae405556d
Tree-SHA512: 9da8f48767ae52d8e8e21c09a40c949cc0838794f1856cc5f58a91acd3f00a3bca818c8082242b3fdc9ca5badb09059570bb3870850d3807b75a8e23b5222da1
By returning an error code if either `FlushUndoFile` or `FlushBlockFile`
fail, the caller now has to explicitly handle block undo file flushing
errors. Before this change such errors were non-explicitly ignored
without a clear rationale.
Besides the call to `FlushUndoFile` in `FlushBlockFile`, ignore its
return code at its call site in `WriteUndoDataForBlock`. There, a failed
flush of the undo data should not be indicative of a failed write.
Add [[nodiscard]] annotations to `FlushUndoFile` such that its return
value is not just ignored in the future.
By returning an error code if `FlushBlockFile` fails, the caller now has
to explicitly handle block file flushing errors. Before this change
such errors were non-explicitly ignored without a clear rationale.
Prior to this patch `FlushBlockFile` may have failed silently in
`Chainstate::FlushStateToDisk`. Improve this with a log line. Also add a
TODO comment to flesh out whether returning early in the case of an
error is appropriate or not. Returning early might be appropriate to
prohibit `WriteBlockIndexDB` from writing a block index entry that does
not refer to a fully flushed block.
Besides `Chainstate::FlushStateToDisk`, `FlushBlockFile` is also called
by `FindBlockPos`. Don't change the abort behavior there, since we don't
want to fail the function if the flushing of already written blocks
fails. Instead, just document it.
