| Age | Commit message (Collapse) | Author |
|---|
|
|
|
|
|
fa6b053b5c964fb35935fa994cb782c0731a56f8 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 fa6b053b5c964fb35935fa994cb782c0731a56f8
glozow:
reACK fa6b053b5c964fb35935fa994cb782c0731a56f8
ismaelsadeeq:
ACK fa6b053b5c964fb35935fa994cb782c0731a56f8
Tree-SHA512: ded2ce3d81bc944b828263534e3178a1e45a914fe8e024f4a14c6561a73e301820944ecc75dd704b3d4221a7a3a5c0597ccab79546250c1197609ee981fe324e
|
|
3b70f7b6156cb110c47a6e482791cf337bb6ad6d doc: fix broken doc/design/multiprocess.md links after #24352 (Ryan Ofsky)
6d43aad742c7ea28303cf2799528188938e7ce32 span: Make Span template deduction guides work in SFINAE context (Ryan Ofsky)
8062c3bdb9dd3062597ed8299e99151b612d32b7 util: Add ArgsManager SetConfigFilePath method (Ryan Ofsky)
441d00c60f0a67889d23f8556190ff99dde488bc interfaces: Rename CalculateBumpFees methods to be compatible with capn'proto (Ryan Ofsky)
156f49d682ef025fb942c997a6c5475e18eef9cf interfaces: Change getUnspentOutput return type to avoid multiprocess segfault (Ryan Ofsky)
4978754c0058bbdfbcd492f25fa49ef211e11d6e interfaces: Add schedulerMockForward method so mockscheduler RPC can work across processes (Ryan Ofsky)
924327eaf3ada45a603e80aa4a3ab38a0f8c8673 interfaces: Fix const virtual method that breaks multiprocess support (Ryan Ofsky)
82a379eca8251c736b4de6e7a2516582641ce397 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 3b70f7b6156cb110c47a6e482791cf337bb6ad6d
naumenkogs:
ACK 3b70f7b6156cb110c47a6e482791cf337bb6ad6d
maflcko:
re-ACK 3b70f7b6156cb110c47a6e482791cf337bb6ad6d 🎆
Tree-SHA512: 2368772b887056ad8a9f84c299cfde76ba45943770e3b5353130580900afa9611302195b899ced7b6e303b11f053ff204cae7c28ff4e12c55562fcc81119ba4c
|
|
fields, remove some external mapTx access
4dd94ca18f6fc843137ffca3e6d3e97e4f19377b [refactor] remove access to mapTx in validation_block_tests (TheCharlatan)
d0cd2e804ec9b278ed9699c2ae48574b1c1613b1 [refactor] rewrite BlockAssembler inBlock and failedTx as sets of txids (glozow)
55b0939cab49d50ca5bc59105b669e379d5e7f6c scripted-diff: rename vTxHashes to txns_randomized (TheCharlatan)
a03aef9cec35b0d03aa63d7e8093f0420cd4b40b [refactor] rewrite vTxHashes as a vector of CTransactionRef (glozow)
938643c3b2b8e7b9aec1df34a2f8a95d616d8dd5 [refactor] remove access to mapTx in validation.cpp (glozow)
333367a9407701b5077e2457b1a6aa8ff5e4934b [txmempool] make CTxMemPoolEntry::lockPoints mutable (glozow)
1bf4855016e777dd8b424fe01750f9e3e97931a2 [refactor] use CheckPackageLimits for checkChainLimits (glozow)
dbc5bdbf595e9dd0330493645ebff0b8696192a3 [refactor] remove access to mapTx.find in mempool_tests.cpp (glozow)
f80909e7a31523d8f197fd650b138b9f228cd13f [refactor] remove access to mapTx in blockencodings_tests.cpp (glozow)
8892d6b744e3cbda2cf93721f573ffa7017bd898 [refactor] remove access to mapTx from rpc/mempool.cpp (glozow)
fad61aa56189f98d97af1d6f70c4eb46b8f98bf0 [refactor] get wtxid from entry instead of vTxHashes (glozow)
9cd8cafb77563243af19c95e396c2fb4fc3758df [refactor] use exists() instead of mapTx.find() (glozow)
14804699e59794e61dcfb02ff1971db96e9a06ce [refactor] remove access to mapTx from policy/rbf.cpp (glozow)
1c6a73abbd1fb773c7d0036beb952b95dde8e38b [refactor] Add helper for retrieving mempool entry (TheCharlatan)
453b4813ebc74859864803e9972b58e4be76a4d6 [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 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b 👝
stickies-v:
re-ACK 4dd94ca18f6fc843137ffca3e6d3e97e4f19377b
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.
|
|
In places where the iterator is only needed for accessing the actual
entry, it should not be required to first retrieve the iterator.
|
|
Instead of reaching into the mapTx data structure, use a helper method
that provides the required vector of CTxMemPoolEntry pointers.
|
|
|
|
|
|
Refactored a variable name to be less confusing
|
|
Changes MiniMinerMempoolEntry order to match the order of the params
elsewhere in the codebase
|
|
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.
|
|
|
|
fa05a726c225dc65dee79367bb67f099ae4f99e6 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 fa05a726c225dc65dee79367bb67f099ae4f99e6.
TheCharlatan:
ACK fa05a726c225dc65dee79367bb67f099ae4f99e6
Tree-SHA512: 4408a094f406e7bf6c1468c2b0798f68f4d952a1253cf5b20bdc648ad7eea4a2c070051fed46d66fd37bce2ce6f85962484a1d32826b7ab8c9baba431eaa2765
|
|
|
|
Also, remove wrong nChainTx comment and cast.
|
|
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
|
|
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
|
|
|
|
non-contiguous, fix feature_init.py file perturbation
d27b9a2248476439ddab7700327f074005a810d5 test: fix feature_init.py file perturbation (Martin Zumsande)
ad66ca1e475d2546dbbda206465307613108a15d 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 d27b9a2248476439ddab7700327f074005a810d5
furszy:
Code ACK d27b9a224
fjahr:
Code review ACK d27b9a2248476439ddab7700327f074005a810d5
Tree-SHA512: 2e54da6030c5813c86bd58f816401e090bb43c5b834764a5e3c0e55dbfe09e423f88042cab823db3742088204b274d4ad2abf58a3832a4b18328b11a30bf7094
|
|
Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
|
|
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`.
|
|
This allows consumers to decide how to handle events from background or
assumedvalid chainstates.
|
|
Removing a snapshot chainstate from disk (and memory) is consistent with
existing reindex operations.
|
|
d8041d4e042957660827313951b18c8dd9a99a16 blockstorage: Return on fatal undo file flush error (TheCharlatan)
f0207e00303a1030eca795ede231e3c0d94df061 blockstorage: Return on fatal block file flush error (TheCharlatan)
5671c15f4520c6dc20e0805fd0b06157ff94bcd7 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 d8041d4e042957660827313951b18c8dd9a99a16
Tree-SHA512: 47ade9b873b15e567c8f60ca538d5a0daf32163e1031be3212a3a45eb492b866664b225f2787c9e40f3e0c089140157d8fd1039abc00c7bdfeec1b52ecd7e219
|
|
This is a refactor.
|
|
This refactor allows to forward some calls to the underlying CAutoFile,
instead of re-implementing the logic in the buffered file.
|
|
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.
|
|
target feerate
f18f9ef4d31c70e2d71ab90a24511692821418c3 Amend bumpfee for inputs with overlapping ancestry (Murch)
2e35e944dab09eff30952233f8dfc0b12c4553d5 Bump unconfirmed parent txs to target feerate (Murch)
3e3e05241128f68cf12f73ee06ff997395643885 coinselection: Move GetSelectionWaste into SelectionResult (Andrew Chow)
c57889da6650715f3e1153b6104bbdae15fcac90 [node] interface to get bump fees (glozow)
c24851be945b2a633ee44ed3c8a501eee5580b62 Make MiniMinerMempoolEntry fields private (Murch)
ac6030e4d8f7d578cd4a8593f41189efca548064 Remove unused imports (Murch)
d2f90c31ef3b8dee5a3e0804ecc62fa1cfec7cd5 Fix calculation of ancestor set feerates in test (Murch)
a1f7d986e0211e54e21a1d4a570e5f15294dca72 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 #9645
Fixes #9864
Fixes #15553
ACKs for top commit:
S3RK:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
ismaelsadeeq:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
achow101:
ACK f18f9ef4d31c70e2d71ab90a24511692821418c3
brunoerg:
crACK f18f9ef4d31c70e2d71ab90a24511692821418c3
t-bast:
ACK https://github.com/bitcoin/bitcoin/pull/26152/commits/f18f9ef4d31c70e2d71ab90a24511692821418c3, I reviewed the latest changes and run e2e tests against eclair, everything looks good :+1:
Tree-SHA512: b65180c4243b1f9d13c311ada7a1c9f2f055d530d6c533b78c2068b50b8c29ac1321e89e85675b15515760d4f1b653ebd9da77b37c7be52d9bc565a3538f0aa6
|
|
from kernel headers
d5067651991f3e6daf456ba13c7036ddc4545352 [refactor] Remove compat.h from kernel headers (TheCharlatan)
36193af47c8dcff53e59498c416b85b59e0d0f91 [refactor] Remove netaddress.h from kernel headers (TheCharlatan)
2b08c55f01996e0b05763f05eac50b83ba9d5a8e [refactor] Add CChainParams member to CConnman (TheCharlatan)
f0d1d8b35c3aa9f2f923f74e3dbbf1e5ece4cd2f [refactor] Add missing includes for next commit (TheCharlatan)
534b314a7401d44f51aabd4565f97be9ee411740 kernel: Move MessageStartChars to its own file (TheCharlatan)
9be330b654cfbd792620295f3867f592059d6a7a [refactor] Define MessageStartChars as std::array (TheCharlatan)
37e2b011136ca1cf00dfb9e575d12f0d035a6a2c [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 d5067651991f3e6daf456ba13c7036ddc4545352.
ajtowns:
utACK d5067651991f3e6daf456ba13c7036ddc4545352
MarcoFalke:
lgtm ACK d5067651991f3e6daf456ba13c7036ddc4545352 🍛
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.
|
|
Follow-up from #27021
|
|
|
|
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>
|
|
|
|
GetType() is only called in tests, so it is unused and can be removed.
|
|
fae405556d56f6f13ce57f69a06b9ec1e825422b scripted-diff: Rename CBlockTreeDB -> BlockTreeDB (MarcoFalke)
faf63039cce40f5cf8dea5a1d24945773c3433a1 Fixup style of moved code (MarcoFalke)
fa65111b99627289fd47dcfaa5197e0f09b8a50e move-only: Move CBlockTreeDB to node/blockstorage (MarcoFalke)
fa8685597e7302fc136f21b6dd3a4b187fa8e251 index: Drop legacy -txindex check (MarcoFalke)
fa69148a0a26c5054dbccdceeac8e117bf449275 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 fae405556d56f6f13ce57f69a06b9ec1e825422b, though I lack historical context to really judge the second commit fa8685597e7302fc136f21b6dd3a4b187fa8e251.
stickies-v:
ACK fae405556d56f6f13ce57f69a06b9ec1e825422b
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.
|
