Age | Commit message (Collapse) | Author |
|
|
|
|
|
Co-authored-by: Dhruv Mehta <856960+dhruv@users.noreply.github.com>
|
|
Most easily reviewed with
git show --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
|
|
This allows consumers to decide how to handle events from background or
assumedvalid chainstates.
|
|
fb6a2ab63e310d8b600352ef41aab6dafccfbff0 scripted-diff: use SER_PARAMS_OPFUNC (Anthony Towns)
5e5c8f86b60a8018e8801fb44bbe56ce97d9deef serialize: add SER_PARAMS_OPFUNC (Anthony Towns)
33203f59b482bddfe0bbe7d497cb8731ce8334a4 serialize: specify type for ParamsWrapper not ref (Anthony Towns)
bf147bfffa1afb11721f30e83eec1fa829f64d5f serialize: move ser_action functions out of global namespace (Anthony Towns)
Pull request description:
Cleanups after #25284:
* ser_action namespacing - https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1316189977
* make reference implicit - https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1316277030
* function notation - https://github.com/bitcoin/bitcoin/pull/25284#issuecomment-1710714821
ACKs for top commit:
MarcoFalke:
lgtm ACK fb6a2ab63e310d8b600352ef41aab6dafccfbff0 💨
TheCharlatan:
ACK fb6a2ab63e310d8b600352ef41aab6dafccfbff0
Tree-SHA512: aacca2ee9cfec360ade6b394606e13d1dfe05bc29c5fbdd48a4e6992bd420312d4ed0d32218d95c560646af326e9977728dc2e759990636298e326947f6f9526
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/WithParams(\(CAddress::V[12]_[A-Z]*\) *, */\1(/g' $(git grep -l 'WithParams' src/)
sed -i 's/WithParams(\(CNetAddr::V[12]\) *, */\1(/g' $(git grep -l 'WithParams' src/)
sed -i 's@\(CNetAddr::V1.CService{}.*\) //@\1 //@' src/test/util/net.cpp
-END VERIFY SCRIPT-
|
|
This is done in preparation to the next commit, but has the nice
effect of removing one further data structure relying on the global
`Params()`.
|
|
Before this commit, there are only two possibly outcomes for the "more" prediction
in Transport::GetBytesToSend():
* true: the transport itself has more to send, so the answer is certainly yes.
* false: the transport has nothing further to send, but if vSendMsg has more message(s)
left, that still will result in more wire bytes after the next
SetMessageToSend().
For the BIP324 v2 transport, there will arguably be a third state:
* definitely not: the transport has nothing further to send, but even if vSendMsg has
more messages left, they can't be sent (right now). This happens
before the handshake is complete.
To implement this, we move the entire decision logic to the Transport, by adding a
boolean to GetBytesToSend(), called have_next_message, which informs the transport
whether more messages are available. The return values are still true and false, but
they mean "definitely yes" and "definitely no", rather than "yes" and "maybe".
|
|
serialization
fa626af3edbe8d98b2de91dd71729ceef90389fb Remove unused legacy CHashVerifier (MarcoFalke)
fafa3fc5a62702da72991497e3270034eb9159c0 test: add tests that exercise WithParams() (MarcoFalke)
fac81affb527132945773a5315bd27fec61ec52f Use serialization parameters for CAddress serialization (MarcoFalke)
faec591d64e40ba7ec7656cbfdda1a05953bde13 Support for serialization parameters (MarcoFalke)
fac42e9d35f6ba046999b2e3a757ab720c51b6bb Rename CSerAction* to Action* (MarcoFalke)
aaaa3fa9477eef9ea72e4a501d130c57b47b470a Replace READWRITEAS macro with AsBase wrapping function (MarcoFalke)
Pull request description:
It seems confusing that picking a wrong value for `ADDRV2_FORMAT` could have effects on consensus. (See the docstring of `ADDRV2_FORMAT`).
Fix this by implementing https://github.com/bitcoin/bitcoin/issues/19477#issuecomment-1147421608 .
This may also help with libbitcoinkernel, see https://github.com/bitcoin/bitcoin/pull/28327
ACKs for top commit:
TheCharlatan:
ACK fa626af3edbe8d98b2de91dd71729ceef90389fb
ajtowns:
ACK fa626af3edbe8d98b2de91dd71729ceef90389fb
Tree-SHA512: 229d379da27308890de212b1fd2b85dac13f3f768413cb56a4b0c2da708f28344d04356ffd75bfcbaa4cabf0b6cc363c4f812a8f1648cff9e436811498278318
|
|
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
|
|
This also cleans up the addrman (de)serialization code paths to only
allow `Disk` serialization. Some unit tests previously forced a
`Network` serialization, which does not make sense, because Bitcoin Core
in production will always `Disk` serialize.
This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
Towns.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
|
|
|
|
This furthers transport abstraction by removing the assumption that a message
can always immediately be converted to wire bytes. This assumption does not hold
for the v2 transport proposed by BIP324, as no messages can be sent before the
handshake completes.
This is done by only keeping (complete) CSerializedNetMsg objects in vSendMsg,
rather than the resulting bytes (for header and payload) that need to be sent.
In SocketSendData, these objects are handed to the transport as permitted by it,
and sending out the bytes the transport tells us to send. This also removes the
nSendOffset member variable in CNode, as keeping track of how much has been sent
is now a responsability of the transport.
This is not a pure refactor, and has the following effects even for the current
v1 transport:
* Checksum calculation now happens in SocketSendData rather than PushMessage.
For non-optimistic-send messages, that means this computation now happens in
the network thread rather than the message handler thread (generally a good
thing, as the message handler thread is more of a computational bottleneck).
* Checksum calculation now happens while holding the cs_vSend lock. This is
technically unnecessary for the v1 transport, as messages are encoded
independent from one another, but is untenable for the v2 transport anyway.
* Statistics updates about per-message sent bytes now happen when those bytes
are actually handed to the OS, rather than at PushMessage time.
|
|
This makes the sending side of P2P transports mirror the receiver side: caller provides
message (consisting of type and payload) to be sent, and then asks what bytes must be
sent. Once the message has been fully sent, a new message can be provided.
This removes the assumption that P2P serialization of messages follows a strict structure
of header (a function of type and payload), followed by (unmodified) payload, and instead
lets transports decide the structure themselves.
It also removes the assumption that a message must always be sent at once, or that no
bytes are even sent on the wire when there is no message. This opens the door for
supporting traffic shaping mechanisms in the future.
|
|
This allows state that is shared between both directions to be encapsulated
into a single object. Specifically the v2 transport protocol introduced by
BIP324 has sending state (the encryption keys) that depends on received
messages (the DH key exchange). Having a single object for both means it can
hide logic from callers related to that key exchange and other interactions.
|
|
fa6286891fa4164510e4fbf4bc214ce3033b2d1b Remove unused includes from wallet.cpp (MarcoFalke)
fa8fdbe22932a4717d2bc4060269da9bff228728 Remove unused includes from blockfilter.h (MarcoFalke)
fad8c36aa9011c3f7b1183f8380577e16a2167a6 move-only: Create src/kernel/mempool_removal_reason.h (MarcoFalke)
fa5760880094c4e4238249f6d1837cd74383cc3a Remove unused includes from txmempool.h (MarcoFalke)
Pull request description:
This makes compilation of wallet.cpp use a few % less memory and time, locally.
Created in the context of https://github.com/bitcoin/bitcoin/issues/28109, but I don't think it is enough to actually fix this problem.
ACKs for top commit:
hebasto:
ACK fa6286891fa4164510e4fbf4bc214ce3033b2d1b, I have reviewed the code and it looks OK.
Tree-SHA512: 06f1120af2a8ef3368dbd9ae747acda88ace2507bd261bcc10341d476a0b3d71c8485377ea6c108b47df3e4c13b7f75a15f486bafa6a8466303168dde16ebbc8
|
|
ChainstateManager
This change makes IsInitialBlockDownload and NotifyHeaderTip functions no
longer tied to individual Chainstate objects. It makes them work with the
ChainstateManager object instead so code is simpler and it is no longer
possible to call them incorrectly with an inactive Chainstate.
This change also makes m_cached_finished_ibd caching easier to reason about,
because now there is only one cached value instead of two (for background and
snapshot chainstates) so the cached IBD state now no longer gets reset when a
snapshot is loaded.
There should be no change in behavior because these functions were always
called on the active ChainState objects.
These changes were discussed previously
https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1246868905 and
https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1237552792 as
possible followups for that PR.
|
|
This removes unused includes, primitives/block found manually, and the
others by iwyu:
blockfilter.h should remove these lines:
- #include <serialize.h> // lines 16-16
- #include <undo.h> // lines 18-18
|
|
91d924ede1b421df31c895f4f43359e453a09ca5 Rename script/standard.{cpp/h} to script/solver.{cpp/h} (Andrew Chow)
bacdb2e208531124e85ed2d4ea2a4b508fbb5088 Clean up script/standard.{h/cpp} includes (Andrew Chow)
f3c9078b4cddec5581e52de5c216ae53984ec130 Clean up things that include script/standard.h (Andrew Chow)
8bbe257bac751859a272ddf52dc0328c1b5a1ede MOVEONLY: Move datacarrier defaults to policy.h (Andrew Chow)
7a172c76d2361fc3cdf6345590e26c79a7821672 Move CTxDestination to its own file (Andrew Chow)
145f36ec81e79d2e391847520364c2420ef0e0e8 Move Taproot{SpendData/Builder} to signingprovider.{h/cpp} (Andrew Chow)
86ea8bed5473f400f7a93fcc455393a574a2f319 Move CScriptID to script.{h/cpp} (Andrew Chow)
b81ebff0d99c45c071b999796b8ae3f0f2517b22 Remove ScriptHash from CScriptID constructor (Andrew Chow)
cba69dda3da0e4fa39cff5ce4dc81d1242fe651b Move MANDATORY_SCRIPT_VERIFY_FLAGS from script/standard.h to policy/policy.h (Anthony Towns)
Pull request description:
Some future work needs to touch things in script/standard.{h/cpp}, however it is unclear if it is safe to do so as they are included in several different places that could effect standardness and consensus. It contains a mix of policy parameters, consensus parameters, and utilities only used by the wallet. This PR breaks up the various components and renames the files to clearly separate everything.
* `CTxDestination` is moved to a new file `src/addresstype.{cpp/h}`
* `TaprootSpendData` and `TaprootBuilder` (and their utility functions and structs) are moved to `SigningProvider` as these are used only during signing.
* `CScriptID` is moved to `script/script.h` to be next to `CScript`.
* `MANDATORY_SCRIPT_VERIFY_FLAGS` is moved to `interpreter.h`
* The parameters `DEFAULT_ACCEPT_DATACARRIER` and `MAX_OP_RETURN_RELAY` are moved to `policy.h`
* `standard.{cpp/h}` is renamed to `solver.{cpp/h}` since that's all that's left in the file after the above moves
ACKs for top commit:
Sjors:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5
ajtowns:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5
MarcoFalke:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5 😇
murchandamus:
ACK 91d924ede1b421df31c895f4f43359e453a09ca5
darosior:
Code review ACK 91d924ede1b421df31c895f4f43359e453a09ca5.
theStack:
Code-review ACK 91d924ede1b421df31c895f4f43359e453a09ca5
Tree-SHA512: d347439890c652081f6a303d99b2bde6c371c96e7f4127c5db469764a17d39981f19884679ba883e28b733fde6142351dd8288c7bc61c379b7eefe7fa7acca1a
|
|
fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0 mempool_entry: improve struct packing (Anthony Towns)
1a118062fbc4ec8f645f4ec4298d869a869c3344 net_processing: Clean up INVENTORY_BROADCAST_MAX constants (Anthony Towns)
6fa49937e488d0924044786c76b42324b659f351 test: Check tx from disconnected block is immediately requestable (glozow)
e4ffabbffacc4b890d393aafcc8286916ef887d8 net_processing: don't add txids to m_tx_inventory_known_filter (Anthony Towns)
6ec1809d33bfc42b80cb6f35625dccd56be8d507 net_processing: drop m_recently_announced_invs bloom filter (Anthony Towns)
a70beafdb22564043dc24fc98133fdadbaf77d8a validation: when adding txs due to a block reorg, allow immediate relay (Anthony Towns)
1e9684f39fba909b3501e9402d5b61f4bf744ff2 mempool_entry: add mempool entry sequence number (Anthony Towns)
Pull request description:
This PR replaces the `m_recently_announced_invs` bloom filter with a simple sequence number tracking the mempool state when we last considered sending an INV message to a node. This saves 33kB per peer (or more if we raise the rate at which we relay transactions over the network, in which case we would need to increase the size of the bloom filter proportionally).
The philosophy here (compare with #18861 and #19109) is that we consider the rate limiting on INV messages to only be about saving bandwidth and not protecting privacy, and therefore after you receive an INV message, it's immediately fair game to request any transaction that was in the mempool at the time the INV message was sent. We likewise consider the BIP 133 feefilter and BIP 37 bloom filters to be bandwidth optimisations here, and treat transactions as requestable if they would have been announced without those filters. Given that philosophy, tracking the timestamp of the last INV message and comparing that against the mempool entry time allows removal of each of `m_recently_announced_invs`, `m_last_mempool_req` and `UNCONDITIONAL_RELAY_DELAY` and associated logic.
ACKs for top commit:
naumenkogs:
ACK fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0
amitiuttarwar:
review ACK fb02ba3c5f5
glozow:
reACK fb02ba3c5f5bcd96b5e3622ef001b8e57ce63fc0
Tree-SHA512: cbba5ee04c86df26b6057f3654c00a2b45ec94d354f4f157a769cecdaa0b509edaac02b3128afba39b023e82473fc5e28c915a787f84457ffe66638c6ac9c2d4
|
|
Remove standard.h from files that don't use anything in it, and include
it in files that do.
|
|
d8f1222ac50f089a0af29eaf8ce0555bad8366ef refactor: Correct dbwrapper key naming (TheCharlatan)
be8f159ac59b9e700cbd3314ed71ebf39bd5b67a build: Remove leveldb from BITCOIN_INCLUDES (TheCharlatan)
c95b37d641b1eed4a62d55ca5342a6ed8c7a1ce7 refactor: Move CDBWrapper leveldb members to their own context struct (TheCharlatan)
c534a615e93452a5f509aaf5f68c600391a98d6a refactor: Split dbwrapper CDBWrapper::EstimateSize implementation (TheCharlatan)
586448888b72f7c87db4dcd30fc4e4044afae13b refactor: Move HandleError to dbwrapper implementation (TheCharlatan)
dede0eef7adb7413f62f5abd68cac8e01635ba4a refactor: Split dbwrapper CDBWrapper::Exists implementation (TheCharlatan)
a5c2eb57484314b04ec94523d14e0ef0c6c46d4f refactor: Fix logging.h includes (TheCharlatan)
84058e0eed9c05bc30984b39131e88ad1425628f refactor: Split dbwrapper CDBWrapper::Read implementation (TheCharlatan)
e4af2408f2ac59788567b6fc8cb3a68fc43da9fe refactor: Pimpl leveldb::Iterator for CDBIterator (TheCharlatan)
ef941ff1281e76308c3e746e592375bec023e9e4 refactor: Split dbwrapper CDBIterator::GetValue implementation (TheCharlatan)
b7a1ab5cb4e60230f62c94efb3a10d07c9af4883 refactor: Split dbwrapper CDBIterator::GetKey implementation (TheCharlatan)
d7437908cdf242626263ba9d5541addcddadc594 refactor: Split dbwrapper CDBIterator::Seek implementation (TheCharlatan)
ea8135de7e617259cda3fc7b1c8e7569d454fd57 refactor: Pimpl leveldb::batch for CDBBatch (TheCharlatan)
b9870c920dc475ec759eaf7339ea42aecba92138 refactor: Split dbwrapper CDBatch::Erase implementation (TheCharlatan)
532ee812a499e13b123af6b8415d8de1f3804f0f refactor: Split dbwrapper CDBBatch::Write implementation (TheCharlatan)
afc534df9adbf5599b286b5dc3531a4b9ac2d056 refactor: Wrap DestroyDB in dbwrapper helper (TheCharlatan)
Pull request description:
Leveldb headers are currently included in the `dbwrapper.h` file and thus available to many of Bitcoin Core's source files. However, leveldb-specific functionality should be abstracted by the `dbwrapper` and does not need to be available to the rest of the code. Having leveldb included in a widely-used header such as `dbwrapper.h` bloats the entire project's header tree.
The `dbwrapper` is a key component of the libbitcoinkernel library. Future users of this library would not want to contend with having the leveldb headers exposed and potentially polluting their project's namespace.
For these reasons, the leveldb headers are removed from the `dbwrapper` by moving leveldb-specific code to the implementation file and creating a [pimpl](https://en.cppreference.com/w/cpp/language/pimpl) where leveldb member variables are indispensable. As a final step, the leveldb include flags are removed from the `BITCOIN_INCLUDES` and moved to places where the dbwrapper is compiled.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587), and more specifically its stage 1 step 3 "Decouple most non-consensus headers from libbitcoinkernel".
ACKs for top commit:
stickies-v:
re-ACK https://github.com/bitcoin/bitcoin/commit/d8f1222ac50f089a0af29eaf8ce0555bad8366ef
MarcoFalke:
ACK d8f1222ac50f089a0af29eaf8ce0555bad8366ef ðŸ”
Tree-SHA512: 0f58309be165af0162e648233451cd80fda88726fc10c0da7bfe4ec2ffa9afe63fbf7ffae9493698d3f39653b4ad870c372eee652ecc90ab1c29d86c387070f3
|
|
1c976c691cc4b20f43071aabf36c7afed1571057 tidy: Integrate bicoin-tidy clang-tidy plugin (fanquake)
7de23cceb8ac13fcc709453ef0fa14fb93c460b0 refactor: fix unterminated LogPrintf()s (fanquake)
0a1029aa2946787bfe91a84de882c2dd0de076e9 lint: remove /* Continued */ markers from codebase (fanquake)
910007995d8603ffc466878856227153a638caff lint: remove lint-logs.py (fanquake)
d86a83d6b8587b0971e66c6910af23dd8c042969 lint: drop DIR_IWYU global (fanquake)
Pull request description:
Demo of integrating the [bitcoin-tidy](https://github.com/theuni/bitcoin-tidy-plugin), [clang-tidy plugin](https://clang.llvm.org/extra/clang-tidy/) written by theuni into our tidy CI job.
The plugin currently has a single check, `bitcoin-unterminated-logprintf`. This would replace our current Python driven, `git-grep`-based, `.cpp` file only, lint-logs linter.
ACKs for top commit:
TheCharlatan:
ACK 1c976c691cc4b20f43071aabf36c7afed1571057
theuni:
ACK 1c976c691cc4b20f43071aabf36c7afed1571057
MarcoFalke:
re-ACK 1c976c691cc4b20f43071aabf36c7afed1571057 ðŸ‘
Tree-SHA512: 725b45c70e431d48e6f276671e05c694e10b6047cae1a31906ac3ee9093bc8105fb226b36a5bac6709557526ca6007222112d66aecec05a574434edc4897e4b8
|
|
with respect to networks
1b52d16d07be3b5d968157913f04d9cd1e2d3678 p2p: network-specific management of outbound connections (Martin Zumsande)
65cff00ceea48ac8a887ffea79aedb4251aa097f test: Add test for outbound protection by network (Martin Zumsande)
034f61f83b9348664d868933dbbfd8f9f8882168 p2p: Protect extra full outbound peers by network (Martin Zumsande)
654d9bc27647fb3797001472e2464dededb45d3f p2p: Introduce data struct to track connection counts by network (Amiti Uttarwar)
Pull request description:
This is joint work with mzumsande.
This is a proposal to diversify outbound connections with respect to reachable networks. The existing logic evaluates peers for connection based purely on the frequency of available addresses in `AddrMan`. This PR adds logic to automatically connect to alternate reachable networks and adds eviction logic that protects one existing connection to each network.
For instance, if `AddrMan` is populated primarily with IPv4 and IPv6 addresses and only a handful of onion addresses, it is likely that we won't establish any automatic outbound connections to Tor, even if we're capable of doing so. For smaller networks like CJDNS, this is even more of an issue and often requires adding manual peers to ensure regularly being connected to the network.
Connecting to multiple networks improves resistance to eclipse attacks for individual nodes. It also benefits the entire p2p network by increasing partition resistance and privacy in general.
The automatic connections to alternate networks is done defensively, by first filling all outbound slots with random addresses (as in the status quo) and then adding additional peers from reachable networks the node is currently not connected to. This approach ensures that outbound slots are not left unfilled while attempting to connect to a network that may be unavailable due to a technical issue or misconfiguration that bitcoind cannot detect.
Once an additional peer is added and we have one more outbound connection than we want, outbound eviction ensures that peers are protected if they are the only ones for their network.
Manual connections are also taken into account: If a user already establishes manual connections to a trusted peer from a network, there is no longer a need to make extra efforts to ensure we also have an automatic connection to it (although this may of course happen by random selection).
ACKs for top commit:
naumenkogs:
ACK 1b52d16d07be3b5d968157913f04d9cd1e2d3678
vasild:
ACK 1b52d16d07be3b5d968157913f04d9cd1e2d3678
Tree-SHA512: 5616c038a5fbb868d4c46c5963cfd53e4599feee25db04b0e18da426d77d22e0994dc4e1da0b810f5b457f424ebbed3db1704f371aa6cad002b3565b20170ec0
|
|
These were uncovered as missing by the next commit.
|
|
Connman uses this new map to keep a count of active OUTBOUND_FULL_RELAY and
MANUAL connections. Unused until next commit.
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's|CBlockTreeDB|BlockTreeDB|g' $( git grep -l CBlockTreeDB )
-END VERIFY SCRIPT-
|
|
The block index (CBlockTreeDB) is required to write and read blocks, so
move it to blockstorage. This allows to drop the txdb.h include from
`node/blockstorage.h`.
Can be reviewed with:
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
|
|
a733dd79e29068ad1e0532ac42a45188a040a7b9 Remove unused function `reliesOnAssumedValid` (Suhas Daftuar)
d4a11abb1972b54f0babdccfbb2fde97ab885933 Cache block index entry corresponding to assumeutxo snapshot base blockhash (Suhas Daftuar)
3556b850221bc0e597d7dd749d4d47ab58dc8083 Move CheckBlockIndex() from Chainstate to ChainstateManager (Suhas Daftuar)
0ce805b632dcb98944a931f758f76f530f5ce5f2 Documentation improvements for assumeutxo (Ryan Ofsky)
768690b7ce551cd403f8e2a099372915f6022ad4 Fix initialization of setBlockIndexCandidates when working with multiple chainstates (Suhas Daftuar)
d43a1f1a2fa35d377c7a9ad7ab92d1ae325bde3d Tighten requirements for adding elements to setBlockIndexCandidates (Suhas Daftuar)
d0d40ea9a6478d81d7531b7cfc52a8bdaa0883d6 Move block-storage-related logic to ChainstateManager (Suhas Daftuar)
3cfc75366e6596942cbc84f354f42dfd7fc5c073 test: Clear block index flags when testing snapshots (Suhas Daftuar)
272fbc370c4e133d31d9f1d34e327cc265c5fad2 Update CheckBlockIndex invariants for chains based on an assumeutxo snapshot (Suhas Daftuar)
10c05710ce1602d932037f72dc6c4bbc3f6f34ba Add wrapper for adding entries to a chainstate's block index candidates (Suhas Daftuar)
471da5f6e74bac71aeffe2ebc5faff145a6cbcea Move block-arrival information / preciousblock counters to ChainstateManager (Suhas Daftuar)
1cfc887d00c5d1d4281107e3b3ff4641c6c34631 Remove CChain dependency in node/blockstorage (Suhas Daftuar)
fe86a7cd480b32463da900db764d2d11a2bea095 Explicitly track maximum block height stored in undo files (Suhas Daftuar)
Pull request description:
This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation.
This PR also fixes a bug in how a chainstate's `setBlockIndexCandidates` was being initialized; it should always have all the HAVE_DATA block index entries that have more work than the chain tip. During startup, we were not fully populating `setBlockIndexCandidates` in some scenarios involving multiple chainstates.
Further, this PR establishes a concept that whenever we have 2 chainstates, that we always know the snapshotted chain's base block and the base block's hash must be an element of our block index. Given that, we can establish a new invariant that the background validation chainstate only needs to consider blocks leading to that snapshotted block entry as potential candidates for its tip. As a followup I would imagine that when writing net_processing logic to download blocks for the background chainstate, that we would use this concept to only download blocks towards the snapshotted entry as well.
ACKs for top commit:
achow101:
ACK a733dd79e29068ad1e0532ac42a45188a040a7b9
jamesob:
reACK a733dd79e29068ad1e0532ac42a45188a040a7b9 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic))
Sjors:
Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9.
ryanofsky:
Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge.
Tree-SHA512: 9ec17746f22b9c27082743ee581b8adceb2bd322fceafa507b428bdcc3ffb8b4c6601fc61cc7bb1161f890c3d38503e8b49474da7b5ab1b1f38bda7aa8668675
|
|
|
|
util/setup_common to util/random
1cd45d4e08c3dfd1d6423620c79169f1404ac12b test: move random.h include header from setup_common.h to cpp (Jon Atack)
1b246fdd145a95f5da479159f5e8eaf5a76bdc3a test: move remaining random test util code from setup_common to random (jonatack)
Pull request description:
and drop the `util/random` dependency on `util/setup_common`. This improves code separation and allows `util/setup_common` to call `util/random` functions without creating a circular dependency, thereby addressing https://github.com/bitcoin/bitcoin/pull/26940#issuecomment-1497266140 by glozow (thanks!)
ACKs for top commit:
MarcoFalke:
lgtm ACK 1cd45d4e08c3dfd1d6423620c79169f1404ac12b 🌂
Tree-SHA512: 6ce63d9103ba9b04eebbd8ad02fe9aa79e356296533404034a1ae88e9b7ca0bc9a5c51fd754b71cf4e7b55b18bcd4d5474b2d588edee3851e3b3ce0e4d309a93
|
|
When simulating a snapshot, remove the HAVE_DATA status for blocks below the
snapshot height, to simulate never having downloaded them at all. This makes
tests more realistic (and more closely match what will happen when using
assumeutxo).
|
|
89ba8905f5c68ae29412f9c4010314c5a113c234 test: indexes, fix on error infinite loop (furszy)
Pull request description:
Coming from https://github.com/bitcoin/bitcoin/pull/28036#issuecomment-1623813703, I thought that we were going to fix it there but seems that got merged without it for some reason.
As index sync failures trigger a shutdown request without notifying `BaseIndex::BlockUntilSyncedToCurrentChain` in any way, we also need to check whether a shutdown was requested or not inside 'IndexWaitSynced'.
Otherwise, any error inside the index sync process will hang the test forever.
ACKs for top commit:
MarcoFalke:
lgtm ACK 89ba8905f5c68ae29412f9c4010314c5a113c234
jamesob:
ACK 89ba890
ryanofsky:
Code review ACK 89ba8905f5c68ae29412f9c4010314c5a113c234. Just comment update since last review
Tree-SHA512: 1f6daf34e51d3fbc802799bfa4ac0ef0d8f774db5f9e2f5d35df18a77679778475c94efc3da1fb723ebaf3583e4075e4a5cbe4a5104ad0c50e2b32076e247b29
|
|
As index sync failures trigger a shutdown request without notifying
BaseIndex::BlockUntilSyncedToCurrentChain in any way, we also need
to check whether a shutdown was requested or not inside 'IndexWaitSynced'.
Otherwise, any error inside the index sync process will hang the test
forever.
|
|
At present, during init, we traverse the chain (once per index)
to confirm that all necessary blocks to sync each index up to
the current tip are present.
To make the process more efficient, we can fetch the oldest block
from the indexers and perform the chain data existence check from
that point only once.
This also moves the pruning violation check to the end of the
'loadinit' thread, which is where the reindex, block loading and
chain activation processes happen.
Making the node's startup process faster, allowing us to remove
the global g_indexes_ready_to_sync flag, and enabling the
execution of the pruning violation verification even when the
reindex or reindex-chainstate flags are enabled (which has being
skipped so far).
|
|
fabed7eb796637c02e3677ebbe183d90b258ba69 test: Restore unlimited timeout in IndexWaitSynced (MarcoFalke)
Pull request description:
The timeout was unlimited before, so just restore that value for now: https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007 .
(Strictly speaking, this is a behavior change for the blockfilterindex and txindex tests, because it only restores the coinstatsindex behavior.)
ACKs for top commit:
ajtowns:
utACK fabed7eb796637c02e3677ebbe183d90b258ba69
mzumsande:
ACK fabed7eb796637c02e3677ebbe183d90b258ba69
furszy:
ACK fabed7eb
Tree-SHA512: 66a878be58bbe53ad8e0c23f05569dd42df688be747551fbd202ada22d20a8285714e58fa2a71664deadb070ddf86cfad88c01042ff95ed26f6b40e4a10cec0a
|
|
validation code.
6eb33bd0c21b3e075fbab596351cacafdc947472 kernel: Add fatalError method to notifications (TheCharlatan)
7320db96f8d2aeff0bc5dc67d8b7b37f5f808990 kernel: Add flushError method to notifications (TheCharlatan)
3fa9094b92c5d37f486b0f8265062d3456796a50 scripted-diff: Rename FatalError to FatalErrorf (TheCharlatan)
edb55e2777063dfeba0a52bbd0b92af8b4688501 kernel: Pass interrupt reference to chainman (TheCharlatan)
e2d680a32d757de0ef8eb836047a0daa1d82e3c4 util: Add SignalInterrupt class and use in shutdown.cpp (TheCharlatan)
Pull request description:
Get rid of all `ShutdownRequested` calls in validation code by introducing an interrupt object that applications can use to cancel long-running kernel operations.
Replace all `AbortNode` calls in validation code with new fatal error and flush error notifications so kernel applications can be notified about failures and choose how to handle them.
---
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/orgs/bitcoin/projects/3 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
The pull request mostly allows dropping the kernel dependency on shutdown.cpp. The only dependency left after this is a `StartShutdown` call which will be removed in followup PR https://github.com/bitcoin/bitcoin/pull/27711. This PR also drops the last reference to the `uiInterface` global in kernel code. The process of moving the `uiInterface` out of the kernel was started in https://github.com/bitcoin/bitcoin/pull/27636.
This pull request contains a subset of patches originally proposed in #27711. It will be part of a series of changes required to make handling of interrupts (or in other words the current shutdown procedure) in the kernel library more transparent and less reliable on global mutable state. The set of patches contained here was originally proposed by @ryanofsky [here](https://github.com/bitcoin/bitcoin/pull/27711#issuecomment-1580779869).
ACKs for top commit:
achow101:
light ACK 6eb33bd0c21b3e075fbab596351cacafdc947472
hebasto:
ACK 6eb33bd0c21b3e075fbab596351cacafdc947472, I have reviewed the code and it looks OK.
ryanofsky:
Code review ACK 6eb33bd0c21b3e075fbab596351cacafdc947472. No changes since last review other than rebase.
Tree-SHA512: 7d2d05fa4805428a09466d43c11ae32946cbb25aa5e741b1eec9cd142e4de4bb311e13ebf1bb125ae490c9d08274f2d56c93314e10f3d69e7fec7445e504987c
|
|
The timeout was unlimited before, so just restore that value for now:
https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1619218007
|
|
|
|
FatalError replaces what previously was the AbortNode function in
shutdown.cpp.
This commit is part of the libbitcoinkernel project and further removes
the shutdown's and, more generally, the kernel library's dependency on
interface_ui with a kernel notification method. By removing interface_ui
from the kernel library, its dependency on boost is reduced to just
boost::multi_index. At the same time it also takes a step towards
de-globalising the interrupt infrastructure.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
|
|
This is done in addition with the following commit. Both have the goal
of getting rid of direct calls to AbortNode from kernel code. This extra
flushError method is added to notify specifically about errors that
arrise when flushing (syncing) block data to disk. Unlike other
instances, the current calls to AbortNode in the blockstorage flush
functions do not report an error to their callers.
This commit is part of the libbitcoinkernel project and further removes
the shutdown's and, more generally, the kernel library's dependency on
interface_ui with a kernel notification method. By removing interface_ui
from the kernel library, its dependency on boost is reduced to just
boost::multi_index. At the same time it also takes a step towards
de-globalising the interrupt infrastructure.
|
|
This and the following commit seek to decouple the libbitcoinkernel
library from the shutdown code. As a library, it should it should have
its own flexible interrupt infrastructure without relying on node-wide
globals.
The commit takes the first step towards this goal by de-globalising
`ShutdownRequested` calls in kernel code.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
|
|
If -acceptstalefeeestimates option is passed stale fee estimates can now
be read when operating in regtest environments.
Additionally, this commit updates all declarations of the CBlockPolicyEstimator
class to include a the second constructor variable.
|
|
|
|
and drop the util/random dependency on util/setup_common.
This improves code separation and avoids creating a circular dependency if
setup_common needs to call the util/random functions.
|
|
interface_ui from validation.
7d3b35004b039f2bd606bb46a540de7babdbc41e refactor: Move system from util to common library (TheCharlatan)
7eee356c0a7fefd70c8de21689efa335f52a69ba refactor: Split util::AnyPtr into its own file (TheCharlatan)
44de325d95447498036479c3112ba741caf45bf6 refactor: Split util::insert into its own file (TheCharlatan)
9ec5da36b62276ae22e348f26f88aaf646357d6d refactor: Move ScheduleBatchPriority to its own file (TheCharlatan)
f871c69191dfe1331861ebcdbadb6bd47e45c8b1 kernel: Add warning method to notifications (TheCharlatan)
4452707edec91c7d7991f486dd41ef3edb4f7fbf kernel: Add progress method to notifications (TheCharlatan)
84d71457e7250ab25c0a11d1ad1c7657197ffd90 kernel: Add headerTip method to notifications (TheCharlatan)
447761c8228d58f948aae7e73ed079c028cacb97 kernel: Add notification interface (TheCharlatan)
Pull request description:
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel".
---
It removes the kernel library's dependency on `util/system` and `interface_ui`. `util/system` contains networking and shell-related code that should not be part of the kernel library. The following pull requests prepared `util/system` for this final step: https://github.com/bitcoin/bitcoin/pull/27419 https://github.com/bitcoin/bitcoin/pull/27254 https://github.com/bitcoin/bitcoin/pull/27238.
`interface_ui` defines functions for a more general node interface and has a dependency on `boost/signals2`. After applying the patches from this pull request, the kernel's reliance on boost is down to `boost::multiindex`.
The approach implemented here introduces some indirection, which makes the code a bit harder to read. Any suggestions for improving or reworking this pull request to make it more concise, or even reworking it into a more proper interface, are appreciated.
ACKs for top commit:
MarcoFalke:
re-ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e (no change) 🎋
stickies-v:
Code Review ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e
hebasto:
re-ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e, only last two commits dropped since my [recent](https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1435394620) review.
Tree-SHA512: c8cfc698dc9d78e20191c444708f2d957501229abe95e5806106d1126fb9c5fbcee686fb55645658c0107ce71f10646f37a2fdf7fde16bbf22cbf1ac885dd08d
|
|
|