926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746 [doc] add release note for TRUC (glozow)
19a9b90617419f68d0f1c90ee115b5220be99a16 use version=3 instead of v3 in debug strings (glozow)
881fac8e609be17eb71bd9a54c0284b304e2e2e2 scripted-diff: change names from V3 to TRUC (glozow)
a573dd261748d2a80560f73db08f7dca788c7fcf [doc] replace mentions of v3 with TRUC (glozow)
089b5757dff39a9a06cdb625aaced9beeb72958d rename mempool_accept_v3.py to mempool_truc.py (glozow)
f543852a89d93441645250c40c3980aeb0c3b664 rename policy/v3_policy.* to policy/truc_policy.* (glozow)
Pull request description:
Adds a release note for TRUC policy which will be live in v28.0.
For clarity, replaces mentions of "v3" with "TRUC" in most places. Suggested in
- https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1629749583
- https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1624500904
I changed error strings from "v3-violation" to "TRUC-violation" but left v3 in the debug strings because I think it might be clearer for somebody who is debugging. Similarly, I left some variables unchanged because I think they're more descriptive this way, e.g. `tx_v3_from_v2_and_v3`. I'm happy to debate places that should or shouldn't be documented differently in this PR, whatever is clearest to everyone.
ACKs for top commit:
instagibbs:
reACK 926b8e39dc
achow101:
ACK 926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
ismaelsadeeq:
Code review ACK 926b8e39dcbc0a3a8a75ef0a29bdca2bf738d746
Tree-SHA512: 16c88add0a29dc6d1236c4d45f34a17b850f6727b231953cbd52eb9f7268d1d802563eadfc8b7928c94ed3d7a615275dd103e57e81439ebf3ba2b12efa1e42af
2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28 test: Remove unnecessary restart in assumeutxo test (Fabian Jahr)
19ce3d407ef546fa50d18b2ffbd67b7417797064 assumeutxo: Check snapshot base block is not marked invalid (Fabian Jahr)
80315c011863d69e7785673283e4c9033fbcd5ac refactor: Move early loadtxoutset checks into ActiveSnapshot (Fabian Jahr)
Pull request description:
This was discovered in a discussion in #29996
If the base block of the snapshot is marked invalid or part of an invalid chain, we currently still load the snapshot and get stuck in a weird state where we have the snapshot chainstate but it will never connect to our valid chain.
While this scenario is highly unlikely to occur on mainnet, it still seems good to prevent this inconsistent state.
The behavior change described above is in the second commit.
The first commit refactors the early checks in the `loadtxoutset` RPC by moving them into `ActivateSnapshot()` in order to have the chance to cover them by unit tests in the future and have a more consistent interface. Previously checks were spread out between `rpc/blockchain.cpp` and `validation.cpp`. In order to be able to return the error message to users of the RPC, the return type of `ActivateSnapshot()` is changed from `bool` to `util::Result`.
The third commit removes an unnecessary restart introduced in #29428.
ACKs for top commit:
mzumsande:
re-ACK 2f9bde6
alfonsoromanz:
Re-ACK 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28. The RPC code looks much cleaner after the refactor. Also, it seems very useful to get the error message in the RPC response rather than having to rely on the logs in some scenarios if you are an RPC user.
achow101:
ACK 2f9bde69f45c7a9fdcf0c65f9e1305391a6f1f28
Tree-SHA512: 5328dd88c3c7be3f1be97c9eef52ac3666c27188c30a798b3e949f3ffcb83be075127c107e4046f7f39f961a79911ea3d61b61f3c11e451b3e4c541c264eeed4
Keep mentions of v3 in debug strings to help people who might not know
that TRUC is applied when version=3.
Also keep variable names in tests, as it is less verbose to keep v3 and v2.
f1478c05458562a9bef5c2ba43959d758e7b4745 mempool: move LoadMempool/DumpMempool to node (Cory Fields)
6d242ff1e9ca02fd8f5cb3ffe82dfb48a52366cc kernel: remove mempool_persist.cpp (Cory Fields)
Pull request description:
DumpMempool/LoadMempool are not necessary for the kernel.
Noticed while working on instantiated logging.
I suppose these could have been left in on purpose, but I'm assuming it was probably just an oversight.
ACKs for top commit:
TheCharlatan:
Re-ACK f1478c05458562a9bef5c2ba43959d758e7b4745
glozow:
ACK f1478c0545
stickies-v:
ACK f1478c05458562a9bef5c2ba43959d758e7b4745
Tree-SHA512: 5825da0cf2e67470524eb6ebe397eb90755a368469a25f184df99ab935b3eb6d89eb802b41a6c3661e869bba3bbfa8ba9d95281bc75ebbf790ec5d9d1f79c66f
55eea003af24169c883e1761beb997e151845225 test: Make blockencodings_tests deterministic (AngusP)
4c99301220ab44e98d0d0e1cc8d774d96a25b7aa test: Add ReceiveWithExtraTransactions Compact Block receive test. (AngusP)
4621e7cc8f8e2b71393a2b30d5dbe56165bfb854 test: refactor: Rename extra_txn to const empty_extra_txn as it is empty in all test cases (AngusP)
Pull request description:
This test uses the `extra_txn` (`vExtraTxnForCompact`) vector of optional orphan/conflicted/etc. transactions to provide transactions to a PartiallyDownloadedBlock that are not otherwise present in the mempool, and check that they are used.
This also covers a former nullptr deref bug that was fixed in #29752 (bf031a517c79cec5b43420bcd40291ab0e9f68a8) where the `extra_txn` vec/circular-buffer was null-initialized and not yet filled when dereferenced in `PartiallyDownloadedBlock::InitData`.
ACKs for top commit:
marcofleon:
Code review ACK 55eea003af24169c883e1761beb997e151845225. I ran the `blockencodings` unit test and no issues with the new test case.
dergoegge:
Code review ACK 55eea003af24169c883e1761beb997e151845225
glozow:
ACK 55eea003af24169c883e1761beb997e151845225
Tree-SHA512: d7909c212bb069e1f6184b26390a5000dcc5f2b18e49b86cceccb9f1ec4f874dd43bc9bc92abd4207c71dd78112ba58400042c230c42e93afe55ba51b943262c
4d81b4de339efbbb68c9785203b699e6e12ecd83 fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read (Vasil Dimov)
b51d75ea97ee0d01ee586e40a30cb68c0bf7ffd3 fuzz: simplify FuzzedSock::m_peek_data (Vasil Dimov)
Pull request description:
Problem:
If `FuzzedSock::Recv(N, MSG_PEEK)` is called then `N` bytes would be
retrieved from the fuzz provider, saved in `m_peek_data` and returned
to the caller (ok).
If after this `FuzzedSock::Recv(M, 0)` is called where `M < N`
then the first `M` bytes from `m_peek_data` would be returned
to the caller (ok), but the remaining `N - M` bytes in `m_peek_data`
would be discarded/lost (not ok). They must be returned by a subsequent
`Recv()`.
To resolve this, only remove the head `N` bytes from `m_peek_data`.
---
This is a followup to https://github.com/bitcoin/bitcoin/pull/30211, more specifically:
https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633199919https://github.com/bitcoin/bitcoin/pull/30211#discussion_r1633216366
ACKs for top commit:
marcofleon:
ACK 4d81b4de339efbbb68c9785203b699e6e12ecd83. Tested this with the I2P fuzz target and there's no loss in coverage. I think overall this is an improvement in the robustness of `Recv` in `FuzzedSock`.
dergoegge:
Code review ACK 4d81b4de339efbbb68c9785203b699e6e12ecd83
brunoerg:
utACK 4d81b4de339efbbb68c9785203b699e6e12ecd83
Tree-SHA512: 73b5cb396784652447874998850e45899e8cba49dcd2cc96b2d1f63be78e48201ab88a76cf1c3cb880abac57af07f2c65d673a1021ee1a577d0496c3a4b0c5dd
a9716c53f05082d6d89ebea51a46d4404efb12d7 rpc: call IsInitialBlockDownload via miner interface (Sjors Provoost)
dda0b0834faf7be7e8938bf63e7bb01cd54a416a rpc: minize getTipHash() calls in gbt (Sjors Provoost)
7b4d3249ced93ec5986500e43b324005ed89502f rpc: call processNewBlock via miner interface (Sjors Provoost)
9e228351e761d8d24413bbc4ac1610b4f3dec2bf rpc: getTransactionsUpdated via miner interface (Sjors Provoost)
64ebb0f97178687517c2060bf6b9931064607888 Always pass options to BlockAssembler constructor (Sjors Provoost)
4bf2e361da1964f7c278b4939967a0e5afde20b0 rpc: call CreateNewBlock via miner interface (Sjors Provoost)
404b01c436122b951e9e06ed26d79dba4651685e rpc: getblocktemplate getTipHash() via Miner interface (Sjors Provoost)
d8a3496b5ad27bea4c79ea0344f595cc1b95f0d3 rpc: call TestBlockValidity via miner interface (Sjors Provoost)
8ecb6816781c7c7f423b501cbb2de3abd7250119 Introduce Mining interface (Sjors Provoost)
Pull request description:
Introduce a `Mining` interface for the `getblocktemplate`, `generateblock` and other mining RPCs to use now, and for Stratum v2 to use later.
Suggested here: https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652
The selection of methods added to the interface is mostly based on what the Template Provider in #29432 uses. It could be expanded further so that `rpc/mining.cpp` no longer needs `EnsureMemPool` and `EnsureChainman`.
This PR should be a pure refactor.
ACKs for top commit:
tdb3:
re ACK a9716c53f05082d6d89ebea51a46d4404efb12d7
itornaza:
Code review and std-tests ACK a9716c53f05082d6d89ebea51a46d4404efb12d7
ryanofsky:
Code review ACK a9716c53f05082d6d89ebea51a46d4404efb12d7 with one minor suggestion in case you update. Only changes since last review were other small changes to the interface.
Tree-SHA512: cf97f87d6e9ed89da3835a0730da3b24a7b14c8605ea221149103a5915e79598cf082a95f2bc88e33f1c450e3d4aad88aed1163a29195acca88bcace055af724
1245d1388b003c46092937def7041917aecec8de netbase: extend CreateSock() to support creating arbitrary sockets (Vasil Dimov)
Pull request description:
Allow the callers of `CreateSock()` to pass all 3 arguments to the `socket(2)` syscall. This makes it possible to create sockets of any domain/type/protocol. In addition to extending arguments, some extra safety checks were put in place.
The need for this came up during the discussion in https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1618837102
ACKs for top commit:
achow101:
ACK 1245d1388b003c46092937def7041917aecec8de
tdb3:
re ACK 1245d1388b003c46092937def7041917aecec8de
theStack:
re-ACK 1245d1388b003c46092937def7041917aecec8de
Tree-SHA512: cc86b56121293ac98959aed0ed77812d20702ed7029b5a043586f46e74295779c5354bb0d5f9e80be6c29e535df980d34c1dbf609064fb7ea3e5ca0f0ed54d6b
6eecba475efd025eb011400af58621ad5823994e net_processing: make MaybePunishNodeFor{Block,Tx} return void (Pieter Wuille)
ae60d485da33f238ed2186799da4e109d4edd3a1 net_processing: remove Misbehavior score and increments (Pieter Wuille)
6457c311977bba3585648e32e3bd5754028aa292 net_processing: make all Misbehaving increments = 100 (Pieter Wuille)
5120ab1478c200b18ee621a6ffa0362f4e991959 net_processing: drop 8 headers threshold for incoming BIP130 (Pieter Wuille)
944c54290d5c081dc433dae7e7941074a3a8b5a7 net_processing: drop Misbehavior for unconnecting headers (Pieter Wuille)
9f66ac7cf1931c4d7c36abbb000b7de306d83a4c net_processing: do not treat non-connecting headers as response (Pieter Wuille)
Pull request description:
So far, discouragement of peers triggers when their misbehavior score exceeds 100 points. Most types of misbehavior increment the score by 100, triggering immediate discouragement, but some types do not. This PR makes all increments equal to either 100 (meaning any misbehavior will immediately cause disconnection and discouragement) or 0 (making the behavior effectively unconditionally allowed), and then removes the logic for score accumulation.
This simplifies the code a bit, but also makes protocol expectations clearer: if a peer misbehaves, they get disconnected. There is no good reason why certain types of protocol violations should be permitted 4 times (howmuch=20) or 9 times (howmuch=10), while many others are never allowed. Furthermore, the distinction between these looks arbitrary.
The specific types of misbehavior that are changed to 100 are:
* Sending us a `block` which does not connect to our header tree (which necessarily must have been unsollicited). [used to be score 10]
* Sending us a `headers` with a non-continuous headers sequence. [used to be score 20]
* Sending us more than 1000 addresses in a single `addr` or `addrv2` message [used to be score 20]
* Sending us more than 50000 invs in a single `inv` message [used to be score 20]
* Sending us more than 2000 headers in a single `headers` message [used to be score 20]
The specific types of misbehavior that are changed to 0 are:
* Sending us 10 (*) separate BIP130 headers announcements that do not connect to our block tree [used to be score 20]
* Sending us more than 8 headers in a single `headers` message (which thus does not get treated as a BIP130 announcement) that does not connect to our block tree. [used to be score 10]
I believe that none of these behaviors are unavoidable, except for the one marked (*) which can in theory happen still due to interaction between BIP130 and variations in system clocks (the max 2 hour in the future rule). This one has been removed entirely. In order to remove the impact of the bug it was designed to deal with, without relying on misbehavior, a separate improvement is included that makes `getheaders`-tracking more accurate.
In another unrelated improvement, this also gets rid of the 8 header limit heuristic to determine whether an incoming non-connecting `headers` is a potential BIP130 announcement, as this rule is no longer needed to prevent spurious Misbehavior. Instead, any non-connecting `headers` is now treated as a potential announcement.
ACKs for top commit:
sr-gi:
ACK [6eecba4](6eecba475e)
achow101:
ACK 6eecba475efd025eb011400af58621ad5823994e
mzumsande:
Code Review ACK 6eecba475efd025eb011400af58621ad5823994e
glozow:
light code review / concept ACK 6eecba475efd025eb011400af58621ad5823994e
Tree-SHA512: e11e8a652c4ec048d8961086110a3594feefbb821e13f45c14ef81016377be0db44b5311751ef635d6e026def1960aff33f644e78ece11cfb54f2b7daa96f946
refactor: CBlockHeaderAndShortTxIDs constructor now always takes an explicit nonce.
test: Make blockencodings_tests deterministic using fixed seed providing deterministic
CBlockHeaderAndShortTxID nonces and dummy transaction IDs.
Fixes very rare flaky test failures, where the ShortIDs of test transactions collide, leading to
`READ_STATUS_FAILED` from PartiallyDownloadedBlock::InitData and/or `IsTxAvailable` giving `false`
when the transaction should actually be available.
* Use a new `FastRandomContext` with a fixed seed in each test, to ensure 'random' uint256s
used as fake prevouts are deterministic, so in-turn test txids and short IDs are deterministic
and don't collide causing very rare but flaky test failures.
* Add new test-only/internal initializer for `CBlockHeaderAndShortTxIDs` that takes a specified
nonce to further ensure determinism and avoid rare but undesireable short ID collisions.
In a test context this nonce is set to a fixed known-good value. Normally it is random, as
previously.
Flaky test failures can be reproduced with:
```patch
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index 695e8d806a..64d635a97a 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -44,7 +44,8 @@ void CBlockHeaderAndShortTxIDs::FillShortTxIDSelector() const {
uint64_t CBlockHeaderAndShortTxIDs::GetShortID(const Wtxid& wtxid) const {
static_assert(SHORTTXIDS_LENGTH == 6, "shorttxids calculation assumes 6-byte shorttxids");
- return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL;
+ // return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0xffffffffffffL;
+ return SipHashUint256(shorttxidk0, shorttxidk1, wtxid) & 0x0f;
}
```
to increase the likelihood of a short ID collision; and running
```shell
set -e;
n=0;
while (( n++ < 5000 )); do
src/test/test_bitcoin --run_test=blockencodings_tests;
done
```
This makes the options argument for BlockAssembler constructor mandatory,
dropping implicit use of ArgsManager. The caller i.e. the Mining
interface implementation now handles this.
In a future Stratum v2 change the Options object needs to be
mofified after arguments have been processed. Specifically
the pool communicates how many extra bytes it needs for
its own outputs (payouts, extra commitments, etc). This will need
to be substracted from what the user set as -blockmaxweight.
Such a change can be implemented in createNewBlock, after
ApplyArgsManOptions.
The fuzz target is generating a large amount of
transactions, but the core of the logic is
ConsumeTxMemPoolEntry making the mempool
entries for adding to the mempool. Since
ConsumeTxMemPoolEntry generates its own transaction
"vsize", we can improve efficiency of the target
by explicitly creating very small transactions,
reducing the hashing and memory burden.
94ed4fbf8e1a396c650b5134d396d6c0be35ce10 Add release note for size 2 package rbf (Greg Sanders)
afd52d8e63ed323a159ea49fd1f10542abeacb97 doc: update package RBF comment (Greg Sanders)
6e3c4394cfadf32c06c8c4732d136ca10c316721 mempool: Improve logging of replaced transactions (Greg Sanders)
d3466e4cc5051c314873dd14ec8f7a88494c0780 CheckPackageMempoolAcceptResult: Check package rbf invariants (Greg Sanders)
316d7b63c97144ba3e21201315c784852210f8ff Fuzz: pass mempool to CheckPackageMempoolAcceptResult (Greg Sanders)
4d15bcf448eb3c4451b63e8f78cc61f3f9f9b639 [test] package rbf (glozow)
dc21f61c72e5a97d974ca2c5cb70b8328f4fab2a [policy] package rbf (Suhas Daftuar)
5da396781589177d4ceb3b4b59c9f309a5e4d029 PackageV3Checks: Relax assumptions (Greg Sanders)
Pull request description:
Allows any 2 transaction package with no in-mempool ancestors to do package RBF when directly conflicting with other mempool clusters of size two or less.
Proposed validation steps:
1) If the transaction package is of size 1, legacy rbf rules apply.
2) Otherwise the transaction package consists of a (parent, child) pair with no other in-mempool ancestors (or descendants, obviously), so it is also going to create a cluster of size 2. If larger, fail.
3) The package rbf may not evict more than 100 transactions from the mempool(bip125 rule 5)
4) The package is a single chunk
5) Every directly conflicted mempool transaction is connected to at most 1 other in-mempool transaction (ie the cluster size of the conflict is at most 2).
6) Diagram check: We ensure that the replacement is strictly superior, improving the mempool
7) The total fee of the package, minus the total fee of what is being evicted, is at least the minrelayfee * size of the package (equivalent to bip125 rule 3 and 4)
Post-cluster mempool this will likely be expanded to general package rbf, but this is what we can safely support today.
ACKs for top commit:
achow101:
ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10
glozow:
reACK 94ed4fbf8e via range-diff
ismaelsadeeq:
re-ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10
theStack:
Code-review ACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10
murchandamus:
utACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10
Tree-SHA512: 9bd383e695964f362f147482bbf73b1e77c4d792bda2e91d7f30d74b3540a09146a5528baf86854a113005581e8c75f04737302517b7d5124296bd7a151e3992
Problem:
If `FuzzedSock::Recv(N, MSG_PEEK)` is called then `N` bytes would be
retrieved from the fuzz provider, saved in `m_peek_data` and returned
to the caller (ok).
If after this `FuzzedSock::Recv(M, 0)` is called where `M < N`
then the first `M` bytes from `m_peek_data` would be returned
to the caller (ok), but the remaining `N - M` bytes in `m_peek_data`
would be discarded/lost (not ok). They must be returned by a subsequent
`Recv()`.
To resolve this, only remove the head `N` bytes from `m_peek_data`.
Allow the callers of `CreateSock()` to pass all 3 arguments to the
`socket(2)` syscall. This makes it possible to create sockets of
any domain/type/protocol.
Instead of having separate warning functions (and globals) for each
different warning that can be raised, encapsulate this logic into
a single class and allow to (un)set any number of warnings.
Introduces behaviour change:
- the `-alertnotify` command is executed for all
`KernelNotifications::warningSet` calls, which now also covers the
`LARGE_WORK_INVALID_CHAIN` warning.
- previously, warnings were returned based on a predetermined order,
e.g. with the "pre-release test build" warning always first. This
is no longer the case, and Warnings::GetMessages() will return
messages sorted by the id of the warning.
Removes warnings.cpp from kernel.
c7376babd19d0c858fef93ebd58338abd530c1f4 doc: Clarify distinction between util and common libraries in libraries.md (Ryan Ofsky)
4f74c59334d496f28e1a5c0d84c412f9020b366f util: Move util/string.h functions to util namespace (Ryan Ofsky)
4d05d3f3b42a41525aa6ec44b90f543dfab53ecf util: add TransactionError includes and namespace declarations (Ryan Ofsky)
680eafdc74021c1e0893c3a62404e607fd4724f5 util: move fees.h and error.h to common/messages.h (Ryan Ofsky)
02e62c6c9af4beabaeea58fb1ea3ad0dc5094678 common: Add PSBTError enum (Ryan Ofsky)
0d44c44ae33434f366229c612d6edeedf7658963 util: move error.h TransactionError enum to node/types.h (Ryan Ofsky)
9bcce2608dd2515dc35a0f0866abc9d43903c795 util: move spanparsing.h to script/parsing.h (Ryan Ofsky)
6dd2ad47922694d2ab84bad4dac9dd442c5df617 util: move spanparsing.h Split functions to string.h (Ryan Ofsky)
23cc8ddff472d259605d7790ba98a1900e77efab util: move HexStr and HexDigit from util to crypto (TheCharlatan)
6861f954f8ff42c87ad638037adae86a5bd89600 util: move util/message to common/signmessage (Ryan Ofsky)
cc5f29fbea15d33e4d1aa95591253c6b86953fe7 build: move memory_cleanse from util to crypto (Ryan Ofsky)
5b9309420cc9721a0d5745b6ad3166a4bdbd1508 build: move chainparamsbase from util to common (Ryan Ofsky)
ffa27af24da81a97d6c4912ae0e10bc5b6f17f69 test: Add check-deps.sh script to check for unexpected library dependencies (Ryan Ofsky)
Pull request description:
Remove `fees.h`, `errors.h`, and `spanparsing.h` from the util library. Specifically:
- Move `Split` functions from `util/spanparsing.h` to `util/string.h`, using `util` namespace for clarity.
- Move remaining spanparsing functions to `script/parsing.h` since they are used for descriptor and miniscript parsing.
- Combine `util/fees.h` and `util/errors.h` into `common/messages.h` so there is a place for simple functions that generate user messages to live, and these functions are not part of the util library.
Motivation for this change is that the util library is a dependency of the kernel, and we should remove functionality from util that shouldn't be called by kernel code or kernel applications. These changes should also improve code organization and make functions easier to discover. Some of these same moves are (or were) part of #28690, but did not help with code organization, or made it worse, so it is better to move them and clean them up in the same PR so code only has to change one time.
ACKs for top commit:
achow101:
ACK c7376babd19d0c858fef93ebd58338abd530c1f4
TheCharlatan:
Re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4
hebasto:
re-ACK c7376babd19d0c858fef93ebd58338abd530c1f4.
Tree-SHA512: 5bcef16c1255463b1b69270548711e7ff78ca0dd34e300b95e3ca1ce52ceb34f83d9ddb2839e83800ba36b200de30396e504bbb04fa02c6d0c24a16d06ae523d
faa41e29d5b90e62179d651f4010272dae685621 fuzz: Use std::span in FuzzBufferType (MarcoFalke)
Pull request description:
The use of `Span` is problematic, because it lacks methods such as `rbegin`, leading to compile failures when used:
```
error: no member named 'rbegin' in 'Span<const unsigned char>'
```
One could fix `Span`, but it seems better to use `std::span`, given that `Span` will be removed anyway in the long term.
ACKs for top commit:
dergoegge:
utACK faa41e29d5b90e62179d651f4010272dae685621
Tree-SHA512: 54bcaf51c83a1b48739cd7f1e8445c6eba0eb04231bce5c35591a47dddb3890ffcaf562cf932930443c80ab0e66950c4619560e6692240de0c52aeef3214facd
193c748e44f8647a056121fc9cbb9c2efbcbfc49 fuzz: add I2P harness (marcofleon)
Pull request description:
Addresses https://github.com/bitcoin/bitcoin/issues/28803. This updated harness sets mock time at the beginning of each iteration and deletes the private key file at the end of each iteration. Mock time is used to make the fuzz test more stable, as `GetTime` is called at points in `i2p`. Deleting the private key file ensures that each iteration is independent from the last. Now, a new key is generated in `i2p` every time, so the fuzzer can eventually make progress through the target code.
Re-working this harness also led me and dergoegge to resolve a couple of issues in `FuzzedSock`, which allows for full coverage of the `i2p` code. Those changes can be seen in https://github.com/bitcoin/bitcoin/pull/30211.
The SAM protocol for interacting with I2P requires some specifc inputs so it's best to use a dictionary when running this harness.
<details>
<summary>I2P dict</summary>
```
"HELLO VERSION"
"HELLO REPLY RESULT=OK VERSION="
"HELLO REPLY RESULT=NOVERSION"
"HELLO REPLY RESULT=I2P_ERROR"
"SESSION CREATE"
"SESSION STATUS RESULT=OK DESTINATION="
"SESSION STATUS RESULT=DUPLICATED_ID"
"SESSION STATUS RESULT=DUPLICATED_DEST"
"SESSION STATUS RESULT=INVALID_ID"
"SESSION STATUS RESULT=INVALID_KEY"
"SESSION STATUS RESULT=I2P_ERROR MESSAGE="
"SESSION ADD"
"SESSION REMOVE"
"STREAM CONNECT"
"STREAM STATUS RESULT=OK"
"STREAM STATUS RESULT=INVALID_ID"
"STREAM STATUS RESULT=INVALID_KEY"
"STREAM STATUS RESULT=CANT_REACH_PEER"
"STREAM STATUS RESULT=I2P_ERROR MESSAGE="
"STREAM ACCEPT"
"STREAM FORWARD"
"DATAGRAM SEND"
"RAW SEND"
"DEST GENERATE"
"DEST REPLY PUB= PRIV="
"DEST REPLY RESULT=I2P_ERROR"
"NAMING LOOKUP"
"NAMING REPLY RESULT=OK NAME= VALUE="
"DATAGRAM RECEIVED DESTINATION= SIZE="
"RAW RECEIVED SIZE="
"NAMING REPLY RESULT=INVALID_KEY NAME="
"NAMING REPLY RESULT=KEY_NOT_FOUND NAME="
"MIN"
"MAX"
"STYLE"
"ID"
"SILENT"
"DESTINATION"
"NAME"
"SIGNATURE_TYPE"
"CRYPTO_TYPE"
"SIZE"
"HOST"
"PORT"
"FROM_PORT"
"TRANSIENT"
"STREAM"
"DATAGRAM"
"RAW"
"MASTER"
"true"
"false"
```
</details>
I'll add this dict to qa-assets later on.
ACKs for top commit:
dergoegge:
tACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
brunoerg:
ACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
vasild:
ACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49
Tree-SHA512: 09ae4b3fa0738aa6f159f4d920493bdbce786b489bc8148e7a135a881e9dba93d727b40f5400c9510e218dd2cfdccc7ce2d3ac9450654fb29c78aac59af92ec3
429ec1aaaaafab150f11e27fcf132a99b57c4fc7 refactor: Rename CTransaction::nVersion to version (Ava Chow)
27e70f1f5be1f536f2314cd2ea42b4f80d927fbd consensus: Store transaction nVersion as uint32_t (Ava Chow)
Pull request description:
Given that the use of a transaction's nVersion is always as an unsigned int, it doesn't make sense to store it as signed and then cast it to unsigned everywhere it is used and displayed.
Since a few alternative implementations have recently been revealed to have made an error with this signedness that would have resulted in consensus failure, I think it makes sense for us to just make this always unsigned to make it clear that the version is treated as unsigned. This would also help us avoid future potential issues with signedness of this value.
I believe that this is safe and does not actually change what transactions would or would not be considered both standard and consensus valid. Within consensus, the only use of the version in consensus is in BIP68 validation which was already casting it to uint32_t. Within policy, although it is used as a signed int for the transaction version number check, I do not think that this change would change standardness. Standard transactions are limited to the range [1, 2]. Negative numbers would have fallen under the < 1 condition, but by making it unsigned, they are still non-standard under the > 2 condition.
Unsigned and signed ints are serialized and unserialized the same way so there is no change in serialization.
ACKs for top commit:
maflcko:
ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 🐿
glozow:
ACK 429ec1aaaa
shaavan:
ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 💯
Tree-SHA512: 0bcd92a245d7d16c3665d2d4e815a4ef28207ad4a1fb46c6f0203cdafeab1b82c4e95e4bdce7805d80a4f4a46074f6542abad708e970550d38a00d759e3dcef1
47f705b33fc1381d96c99038e2110e6fe2b2f883 tests: add fuzz tests for BitSet (Pieter Wuille)
59a6df6bd584701f820ad60a10d9d477bf0236b5 util: add BitSet (Pieter Wuille)
Pull request description:
Extracted from #30126.
This introduces the `BitSet` data structure, inspired by `std::bitset`, but with a few features that cannot be implemented on top without efficiency loss:
* Finding the first set bit (`First`)
* Finding the last set bit (`Last`)
* Iterating over all set bits (`begin` and `end`).
And a few other operators/member functions that help readability for #30126:
* `operator-` for set subtraction
* `Overlaps()` for testing whether intersection is non-empty
* `IsSupersetOf()` for testing (non-strict) supersetness
* `IsSubsetOf()` for testing (non-strict) subsetness
* `Fill()` to construct a set with all numbers from 0 to n-1, inclusive
* `Singleton()` to construct a set with one specific element.
Everything is tested through a simulation-based fuzz test that compares the behavior with normal `std::bitset` equivalent operations.
ACKs for top commit:
instagibbs:
ACK 47f705b33f
achow101:
ACK 47f705b33fc1381d96c99038e2110e6fe2b2f883
cbergqvist:
re-ACK 47f705b33fc1381d96c99038e2110e6fe2b2f883
theStack:
Code-review ACK 47f705b33fc1381d96c99038e2110e6fe2b2f883
Tree-SHA512: e451bf4b801f193239ee434b6b614f5a2ac7bb49c70af5aba24c2ac0c54acbef4672556800e4ac799ae835632bdba716209c5ca8c37433a6883dab4eb7cd67c1
5bc2077e8f592442b089affdf0b5795fbc053bb8 validation: allow to specify frequency for -checkblockindex (Martin Zumsande)
d5a631b9597e5029a5048d9b8ad84ea4536bbac0 validation: improve performance of CheckBlockIndex (Martin Zumsande)
32c80413fdb063199f3bee719c4651bd63f05fce bench: add benchmark for checkblockindex (Martin Zumsande)
Pull request description:
`CheckBlockIndex() ` are consistency checks that are currently enabled by default on regtest.
The function is rather slow, which is annoying if you
* attempt to run it on other networks, especially if not fully synced
* want to generate a long chain on regtest and see block generation slow down because you forgot to disable `-checkblockindex` or don't know it existed.
One reason why it's slow is that in order to be able to traverse the block tree depth-first from genesis, it inserts pointers to all block indices into a `std::multimap` - for which inserts and lookups become slow once there are hundred thousands of entries.
However, typically the block index is mostly chain-like with just a few forks so a multimap isn't really needed for the most part. This PR suggests to store the block indices of the chain ending in the best header in a vector instead, and store only the rest of the indices in a multimap. This does not change the actual consistency checks that are being performed for each index, just the way the block index tree is stored and traversed.
This adds a bit of complication to make sure each block is visited (note that there are asserts that check it), making sure that the two containers are traversed correctly, but it speeds up the function considerably:
On master, a single invocation of `CheckBlockIndex` takes ~1.4s on mainnet for me (4.9s on testnet which has >2.4 million blocks).
With this branch, the runtime goes down to ~0.27s (0.85s on testnet).This is a speedup by a factor ~5.
ACKs for top commit:
achow101:
ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8
furszy:
ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8
ryanofsky:
Code review ACK 5bc2077e8f592442b089affdf0b5795fbc053bb8. Just added suggested assert and simplification since last review
Tree-SHA512: 6b9c3e3e5069d6152b45a09040f962380d114851ff0f9ff1771cf8cad7bb4fa0ba25cd787ceaa3dfa5241fb249748e2ee6987af0ccb24b786a5301b2836f8487
09ef322acc0a88a9e119f74923399598984c68f6 [[refactor]] Check CTxMemPool options in constructor (TheCharlatan)
Pull request description:
The tests should run the same checks on the mempool options that the init code also applies. The downside to this patch is that the log line may now be printed more than once in the for loop.
This was originally noticed here https://github.com/bitcoin/bitcoin/pull/25290#discussion_r900272797.
ACKs for top commit:
stickies-v:
re-ACK 09ef322acc0a88a9e119f74923399598984c68f6 . Fixed unreachable assert and updated docstring, and also added an exception for "-maxmempool must be at least " in the `tx_pool` fuzz test, which makes sense when looking at how the mempool options are constructed in `SetMempoolConstraints`.
achow101:
ACK 09ef322acc0a88a9e119f74923399598984c68f6
ryanofsky:
Code review ACK 09ef322acc0a88a9e119f74923399598984c68f6. Just fuzz test error checking fix and updated comment since last review
Tree-SHA512: eb3361411c2db70be17f912e3b14d9cb9c60fb0697a1eded952c3b7e8675b7d783780d45c52e091931d1d80fe0f0280cee98dd57a3100def13af20259d9d1b9e
d1581c6048478cf70c5fb9ec5ebc178f16b376b8 test: doc: fix units in tx size standardness test (s/vbytes/weight units) (Sebastian Falbesoner)
Pull request description:
This small fixup PR is a late follow-up for #17947 (commit 4537ba5f21ad8afb705325cd8e15dd43877eb28f), where the wrong units has been used in the comments for the large tx composition.
ACKs for top commit:
tdb3:
ACK d1581c6048478cf70c5fb9ec5ebc178f16b376b8
ismaelsadeeq:
ACK d1581c6048478cf70c5fb9ec5ebc178f16b376b8
glozow:
ACK d1581c6048478cf70c5fb9ec5ebc178f16b376b8
Tree-SHA512: ea2de42174f9dca0608275ea377c852ebddc5a04a2b32248ce808aea33d7e00cdee3a225b24c0cf426c69646cccbbc31273c62f7bc1647bb3443a61de3b15670
f68cba29b3be0dec7877022b18a193a3b78c1099 blockman: Replace m_reindexing with m_blockfiles_indexed (Ryan Ofsky)
1b1c6dcca0cc891bd35d29b61628c39098cd94ce test: Add functional test for continuing a reindex (TheCharlatan)
201c1a92824c71ae646d5bba9963871b1d704cc1 indexes: Don't wipe indexes again when already reindexing (TheCharlatan)
804f09dfa116300914e2aeef05ed9710dd504e8c kernel: Add less confusing reindex options (Ryan Ofsky)
e17255322378076edce3ef6f06cd36ca58d2e236 validation: Remove needs_init from LoadBlockIndex (TheCharlatan)
533eab7d67d78f217f74909662133086b79ea808 bugfix: Streamline setting reindex option (TheCharlatan)
Pull request description:
When restarting `bitcoind` during an ongoing reindex without setting the `-reindex` flag again, the block and coins db is left intact, but any data from the optional indexes is discarded. While not a bug per se, wiping the data again is
wasteful, both in terms of having to write it again, as well as potentially leading to longer startup times. So keep the index data instead when continuing a prior reindex.
Also includes a bugfix and smaller code cleanups around the reindexing code. The bug was introduced in b47bd959207e82555f07e028cc2246943d32d4c3: "kernel: De-globalize fReindex".
ACKs for top commit:
stickies-v:
ACK f68cba29b3be0dec7877022b18a193a3b78c1099
fjahr:
Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099
furszy:
Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099
ryanofsky:
Code review ACK f68cba29b3be0dec7877022b18a193a3b78c1099. Only changes since last review were cherry-picking suggested commits that rename variables, improving comments, and making some tweaks to test code.
Tree-SHA512: b252228cc76e9f1eaac56d5bd9e4eac23408e0fc04aeffd97a85417f046229364673ee1ca7410b9b6e7b692b03f13ece17c42a10176da0d7e975a8915deb98ca
15796d4b61342f75548b20a18c670ed21d102ba8 build: warn on self-assignment (Cory Fields)
53372f21767be449bb452fc3f5fe7f16286ae371 refactor: disable self-assign warning for tests (Cory Fields)
Pull request description:
Belt-and suspenders after #30234. Self-assignment should be safe _and_ discouraged.
We used to opt out of this warning because something deep in our serialization/byteswapping code could self-assign, but that doesn't appear to be the case anymore.
ACKs for top commit:
maflcko:
ACK 15796d4b61342f75548b20a18c670ed21d102ba8
fanquake:
ACK 15796d4b61342f75548b20a18c670ed21d102ba8 - not a huge fan of inline pragma usage, but this seems fine, given it's to work around an already-fixed compiler bug, and we'll only be carrying it for a shortish time in any case.
Tree-SHA512: 1f95f7c730b974ad1da55ebd381040bac312f2f380fff9d569ebab91d7c1963592a84d1613d81d96238c6f5a66aa40deebba68a76f6b24b02150d0a77c769654
1f6ab1215bbb1f8a5f1743c3c413b95ad08090df minor: remove unnecessary semicolons from RPC content type examples (Matthew Zipkin)
b22529529823c0cb5916ac318c8536e9107b7e78 test: use json-rpc 2.0 in all functional tests by default (Matthew Zipkin)
391843b0297db03d71a8d88ab77609e2ad230bf2 bitcoin-cli: use json-rpc 2.0 (Matthew Zipkin)
d39bdf339772166a5545ae811e58b7764af093a8 test: remove unused variable in interface_rpc.py (Matthew Zipkin)
0ead71df8c83a2f9eae1220544ec84dcf38a0326 doc: update and link for JSON-RPC 2.0 (Matthew Zipkin)
Pull request description:
This is a follow-up to #27101.
- Addresses [post-merge comments ](https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1606723428)
- bitcoin-cli uses JSON-RPC 2.0
- functional tests use JSON-RPC 2.0 by default (exceptions are in the regression tests added by #27101)
ACKs for top commit:
tdb3:
ACK 1f6ab1215bbb1f8a5f1743c3c413b95ad08090df
cbergqvist:
ACK 1f6ab1215bbb1f8a5f1743c3c413b95ad08090df
Tree-SHA512: 49bf14c70464081280216ece538a2f5ec810bac80a86a83ad3284f0f1b017edf755a1a74a45be279effe00218170cafde7c2de58aed07097a95c2c6b837a6b6c
In order to ensure that the change of nVersion to a uint32_t in the
previous commit has no effect, rename nVersion to version in this commit
so that reviewers can easily spot if a spot was missed or if there is a
check somewhere whose semantics have changed.
Drop confusing kernel options:
BlockManagerOpts::reindex
ChainstateLoadOptions::reindex
ChainstateLoadOptions::reindex_chainstate
Replacing them with more straightforward options:
ChainstateLoadOptions::wipe_block_tree_db
ChainstateLoadOptions::wipe_chainstate_db
Having two options called "reindex" which did slightly different things
was needlessly confusing (one option wiped the block tree database, and
the other caused block files to be rescanned). Also the previous set of
options did not allow rebuilding the block database without also
rebuilding the chainstate database, when it should be possible to do
those independently.
30a01134cdec37e7467fcd6eee8b0ae3890a131c [doc] update bips.md for 431 (glozow)
9dbe6a03f0d6e70ccdf8e8715f888c0c17216bee [test] wallet uses CURRENT_VERSION which is 2 (glozow)
539404fe0fc0346b3aa77c330b38a5a0ad6565b2 [policy] make v3 transactions standard (glozow)
052ede75aff5c9f3a0a422ef413852eabeecc665 [refactor] use TRUC_VERSION in place of 3 (glozow)
Pull request description:
Make `nVersion=3` (which is currently nonstandard on mainnet) standard.
Note that we will treat these transactions as Topologically Restricted Until Confirmation (TRUC). Spec is in BIP 431 and implementation is in #28948, #29306, and #29873
See #27463 for overall project tracking, and #29319 for information about relevance to cluster mempool.
ACKs for top commit:
sdaftuar:
utACK 30a01134c
achow101:
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c
instagibbs:
utACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c
murchandamus:
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c
ismaelsadeeq:
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c 🛰️
Tree-SHA512: 2a4aec0442c860e792a061d83e36483c1f1b426f946efbdf664c8db97a596e498b535707e1d3a900218429486ea69fd4552e3d476526a6883cbd5556c6534b48
