With subpackage evaluation and de-duplication, it's not always the
entire package that is used in CheckFeerate. To be more helpful to the
caller, specify which transactions were included in the evaluation and
what the feerate was.
Instead of PCKG_POLICY (which is supposed to be for package-wide
errors), use PCKG_TX.
fa7ba926300f44b350d04262e569cc607e4991d8 fuzz: Avoid utxo_total_supply timeout (MarcoFalke)
Pull request description:
Looks like this still may take a long time to run large fuzz inputs. Thus, reduce it further, but still allow it to catch the regression, if re-introduced:
```diff
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index f949655909..4bdd15c5ee 100644
--- a/src/consensus/tx_check.cpp
+++ b/src/consensus/tx_check.cpp
@@ -40,7 +40,7 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
std::set<COutPoint> vInOutPoints;
for (const auto& txin : tx.vin) {
if (!vInOutPoints.insert(txin.prevout).second)
- return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate");
+ {}//return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate");
}
if (tx.IsCoinBase())
```
This is the second take, see https://github.com/bitcoin/bitcoin/pull/27780. If in the future it still times out, I think the fuzz test can just be removed.
Example input:
```
JREROy5pcnAgQyw7IC4ODg4ODg4ODg4O0dEODg4ODg4ZDg4ODg4ODg4ODg7RDg4ODg4ODg4O0dEODg4ODg4ODg4ODg7R0Q4ODg4ODg4ODtHRDg4ODtHR0dEODg4O0dEODg7R0Q4ODg4ODg4ODtHRDg4ODg4ODg4ODg4O0dEODg4ODg4ODg7R0Q4ODg7R0Q4O0dEODg4ODg4ODg4ODg7R0Q4ODg4ODtHRDg4ODtHR
ACKs for top commit:
dergoegge:
ACK fa7ba926300f44b350d04262e569cc607e4991d8
brunoerg:
utACK fa7ba926300f44b350d04262e569cc607e4991d8
Tree-SHA512: 154a4895834babede6ce7b775562a7026637af1097e53e55676e92f6cf966ae0c092300ebf7e51a397eebd11f7b41d020586663e781f70d084efda1c0fe851b4
5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7 test: remove custom rpc timeout for `wallet_miniscript.py`, reorder in test_runner (Sebastian Falbesoner)
f811a24421fe102a96ab8f75427cc6a3c5503dc3 wallet: cache descriptor ID to avoid repeated descriptor string creation (Sebastian Falbesoner)
Pull request description:
Right now a wallet descriptor is converted to its string representation (via `Descriptor::ToString`) repeatedly at different instances:
- on finding a `DescriptorScriptPubKeyMan` for a given descriptor (`CWallet::GetDescriptorScriptPubKeyMan`, e.g. used by the `importdescriptors` RPC); the string representation is created once for each spkm in the wallet and at each iteration again for the searched descriptor (`DescriptorScriptPubKeyMan::HasWalletDescriptor`)
- whenever `DescriptorScriptPubKeyMan::GetID()` is called, e.g. in `TopUp` or any instances where a descriptor is written to the DB to determine the database key, also at less obvious places like `FastWalletRescanFilter` etc.
As there is no good reason to calculate a fixed descriptor's string/ID more than once, add the ID as a field to `WalletDescriptor` and calculate it immediately at initialization (or deserialization). `HasWalletDescriptor` is changed to compare the spkm's and searched descriptor's ID instead of the string to take use of that.
This speeds up the functional test `wallet_miniscript.py` by a factor of 5-6x on my machine (3m30.95s on master vs. 0m38.02s on PR). The recently introduced "max-size TapMiniscript" test-case introduced a descriptor that takes 2-3 seconds to create a string representation, so the repeated calls to that were significantly hurting the performance.
Fixes https://github.com/bitcoin/bitcoin/issues/28800.
ACKs for top commit:
Sjors:
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
S3RK:
Code Review ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
achow101:
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
BrandonOdiwuor:
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
Tree-SHA512: 98b43963a5dde6055bb26cecd3b878dadd837d6226af4c84142383310495da80b3c4bd552e73b9107f2f2ff1c11f5e18060c6fd3d9e44bbd5224114c4d245c1c
3c208cc05ea9efb145c956e70f80efd8b027ff33 Add offline signing tutorial (Brandon Odiwuor)
Pull request description:
This PR adds offline signing tutorial. Fixes https://github.com/bitcoin/bitcoin/issues/9492
Although there currently exists tutorials on external-signer and on multisig implemented on #24519 . The external-signer tutorial assumes a connected device and the multisig tutorial is only for multisig transactions and does not include using an offline wallet
- The tutorial uses signet(instead of regtest) to be as close as possible to mainnet
ACKs for top commit:
achow101:
ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33
willcl-ark:
ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33
pinheadmz:
ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33
Zero-1729:
ACK 3c208cc05ea9efb145c956e70f80efd8b027ff33
Tree-SHA512: c1686043d9e9ed440e78d219a6b18d58d62efd05bdd535e74194d8cc2db0a91e94c6c619106453120a137e47220cf3ab27af3214e861f4e5cc419a73a8704dd6
With package validation rules, transactions that fail individually may
sometimes be eligible for reconsideration if submitted as part of a
(different) package. For now, that includes trasactions that failed for
being too low feerate. Add a new TxValidationResult type to distinguish
these failures from others. In the next commits, we will abort package
validation if a tx fails for any other reason. In the future, we will
also decide whether to cache failures in recent_rejects based on this
result (we won't want to reject a package containing a transaction that
was rejected previously for being low feerate).
Package validation also sometimes elects to skip some transactions when
it knows the package will not be submitted in order to quit sooner. Add
a result to specify this situation; we also don't want to cache these
as rejections.
Right now a wallet descriptor is converted to it's string representation
(via `Descriptor::ToString`) repeatedly at different instances:
- on finding a `DescriptorScriptPubKeyMan` for a given descriptor
(`CWallet::GetDescriptorScriptPubKeyMan`, e.g. used by the
`importdescriptors` RPC); the string representation is created once
for each spkm in the wallet and at each iteration again for
the searched descriptor (`DescriptorScriptPubKeyMan::HasWalletDescriptor`)
- whenever `DescriptorScriptPubKeyMan::GetID()` is called, e.g. in
`TopUp` or any instances where a descriptor is written to the DB
to determine the database key etc.
As there is no good reason to calculate a fixed descriptor's string/ID
more than once, add the ID as a field to `WalletDescriptor` and
calculate it immediately at initialization (or deserialization).
`HasWalletDescriptor` is changed to compare the spkm's and searched
descriptor's ID instead of the string to take use of that.
This speeds up the functional test `wallet_miniscript.py` by a factor of
5-6x on my machine (3m30.95s on master vs. 0m38.02s on PR). The recently
introduced "max-size TapMiniscript" test-case introduced a descriptor
that takes 2-3 seconds to create a string representation, so the
repeated calls to that were significantly hurting the performance.
3333f14efac815ba3c885398a6795c7e8ce68d08 depends: Bump to capnproto-c++-1.0.1 (MarcoFalke)
Pull request description:
Reasons:
* Debian is starting to ship this version in Trixie (https://packages.debian.org/trixie/capnproto), which will likely become the version shipped with Ubuntu 24.04 LTS. So testing with this version will help to find any issues before real users start to use those distro packages.
* The feature is currently experimental, so bumping the version shouldn't cause any production issues.
* With multiprocess begin a priority project for 27.0, it seems better to do build system changes/bumps early, rather than later, to allow for more time testing them.
ACKs for top commit:
TheCharlatan:
Re-ACK 3333f14efac815ba3c885398a6795c7e8ce68d08
fanquake:
ACK 3333f14efac815ba3c885398a6795c7e8ce68d08 - the response from upstream is that [if we submit a PR, they can take a look](https://github.com/capnproto/capnproto/issues/1833#issuecomment-1792582206), so if anyone would like this to work for Windows, I'd suggest sending a patch.
ryanofsky:
Code review ACK 3333f14efac815ba3c885398a6795c7e8ce68d08
Tree-SHA512: 7d53ad1536f042ab43dbc7847126b826e7fc76694f173c348b835fd1067b8f3dd682c5bcb4887f09ee85bab69130721cd7f8fb96b2e82053d4e28bd5c38bdc5f
5bd1b8d4f1ab4dd947c5e93712ba47e14a0fe2da ci: Drop no longer needed "Fix Visual Studio installation" step (Hennadii Stepanov)
Pull request description:
The underlying issue has been [fixed](https://github.com/actions/runner-images/pull/8686) in the image version 20231029.
ACKs for top commit:
maflcko:
lgtm ACK 5bd1b8d4f1ab4dd947c5e93712ba47e14a0fe2da
Tree-SHA512: d0efef3086a147d863c9b5f45ba1142c6e7cc65e47d685b2094211e58036315fb7562253b7d7172b527fa1ded4b5a86634ba7c151e761ec20fe948145aff83fe
The `-zapwallettxes` functionality has been removed in v0.21.0
(see commit 3340dbadd38f5624642cf0e14dddbe6f83a3863b / PR #19671),
with the parameter being kept as hidden option, to inform users via
an exit error that `abandontransaction` should be used instead.
As any guides that still suggest to use `-zapwallettxes` would refer to
a Bitcoin Core version that is EOL since many years (i.e. <= v0.20.x),
it is highly unlikely that the error caused by the option is still
relevant for any user, hence it seems fine to remove it now.
d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9 [test] MiniMiner::Linearize and manual construction (glozow)
dfd6a3788c35be121eba1ad84f20effadcb7e7dc [refactor] unify fee amounts in miniminer_tests (glozow)
f4b1b24a3bcd0199a6d2e828ad46033e1368336e [MiniMiner] track inclusion order and add Linearize() function (glozow)
004075963f12f17c3c399d81e3b48d6a8151aebd [test] add case for MiniMiner working with negative fee txns (glozow)
fe6332c0badf07e99044b00084fc6b07f735a051 [MiniMiner] make target_feerate optional (glozow)
5a83f55c96661a886dd6f5231920b2f730cf6773 [MiniMiner] allow manual construction with non-mempool txns (glozow)
e3b2e630b219ca15fe0b2640ca422712c86ac33d [refactor] change MiniMinerMempoolEntry ctor to take values, update includes (glozow)
4aa98b79b266cd526efa577762b0bcfccbdeda11 [lint] update expected boost includes (glozow)
Pull request description:
This is part of #27463. It splits off the `MiniMiner`-specific changes from #26711 for ease of review, as suggested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.
- Allow using `MiniMiner` on transactions that aren't in the mempool.
- Make `target_feerate` param of `BuildMockTemplate` optional, meaning "don't stop building the template until all the transactions have been selected."
- Add clarification for how this is different from `target_feerate=0` (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1377019133)
- Track the order in which transactions are included in the template to get the "linearization order" of the transactions.
- Tests
Reviewers can take a look at #26711 to see how these functions are used to linearize the `AncestorPackage` there.
ACKs for top commit:
TheCharlatan:
ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9
kevkevinpal:
reACK [d9cc99d](d9cc99d04e)
achow101:
re-ACK d9cc99d04e813caed51c5f7b6ebdc4c39c8823c9
Tree-SHA512: 32b80064b6679536ac573d674825c5ca0cd6245e49c2fd5eaf260dc535335a57683c74ddd7ce1f249b5b12b2683de4362a7b0f1fc0814c3b3b9f14c682665583
b5a60abe8783852f5b31bc1e63b5836530410e65 MOVEONLY: CleanupTemporaryCoins into its own function (glozow)
10c0a8678cd28e7f0715e6cfa3e651903e4ad4aa [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow)
6ff647a7e0d85040a6033047c5cf84f8f22b1c65 scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow)
da9aceba217bbded6909f06144eaa1e1a4ebcb69 [refactor] move package checks into helper functions (glozow)
Pull request description:
This is part of #27463. It splits off the more trivial changes from #26711 for ease of review, as requested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.
- Split package sanitization in policy/packages.h into helper functions
- Add some tests for its quirks (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1340521597)
- Rename `CheckPackage` to `IsPackageWellFormed`
- Improve the `CreateValidTransaction` unit test utility to:
- Configure the target feerate and return the fee paid
- Signal BIP125 on transactions to enable RBF tests
- Allow the specification of multiple inputs and outputs
- Move `CleanupTemporaryCoins` into its own function to be reused later without duplication
ACKs for top commit:
dergoegge:
Code review ACK b5a60abe8783852f5b31bc1e63b5836530410e65
instagibbs:
ACK b5a60abe8783852f5b31bc1e63b5836530410e65
Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec
fcb3069fa307942cf7f3edabcda1be96d615c91f Use CheckPackageMempoolAcceptResult for package evaluation fuzzing (Greg Sanders)
34088d6c9ed4ed99bb6b7fc83795da01ec9f3c97 [test util] CheckPackageMempoolAcceptResult for sanity-checking results (glozow)
651fa404e454e31f8e9d830aa292eb3b456b54fb fuzz: tx_pool checks ATMP result invariants (Greg Sanders)
Pull request description:
Poached from https://github.com/bitcoin/bitcoin/pull/26711 since that PR is being split apart, and modified to match current behavior.
ACKs for top commit:
glozow:
reACK fcb3069fa307942cf7f3edabcda1be96d615c91f, only whitespace changes
dergoegge:
ACK fcb3069fa307942cf7f3edabcda1be96d615c91f
Tree-SHA512: abd687e526d8dfc8d65b3a873ece8ca35fdcbd6b0f7b93da6a723ef4e47cf85612de819e6f2b8631bdf897e1aba27cdd86f89b7bd85fc3356e74be275dcdf8cc
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.
bb91131d545d986aab81c4bb13676c4520169259 doc: remove out-of-date external link in src/util/strencodings.h (Jon Atack)
7d494a48ddf4248ef3b1753b6e7f2eeab3a8ecb7 refactor: use string_view to pass string literals to Parse{Hash,Hex} (Jon Atack)
Pull request description:
as `string_view` is optimized to be trivially copiable, whereas the current code creates a `std::string` copy at each call.
These utility methods are called by quite a few RPCs and tests, as well as by each other.
```
$ git grep "ParseHashV\|ParseHashO\|ParseHexV\|ParseHexO" | wc -l
61
```
Also remove an out-of-date external link.
ACKs for top commit:
jonatack:
Rebased per `git range-diff c9273f6 b94581a bb91131` for an include header from the merge of https://github.com/bitcoin/bitcoin/pull/28230. Should be trivial to re-ACK.
maflcko:
lgtm ACK bb91131d545d986aab81c4bb13676c4520169259
ns-xvrn:
ACK bb91131d54
achow101:
ACK bb91131d545d986aab81c4bb13676c4520169259
brunoerg:
crACK bb91131d545d986aab81c4bb13676c4520169259
Tree-SHA512: 9734fe022c9e43fd93c23a917770d332dbbd3132c80a234059714c32faa6469391e59349954749fc86c4ef0b18d5fd99bf8f4b7b82d9f799943799c1253272ae
376dc2cfb32806a8aa450589effe4d384e648398 test: add coverage to rpc_blockchain.py (kevkevin)
Pull request description:
Included a test that checks the functionality of setting
the first param of getnetworkhashps to negative value returns
the average network hashes per second from the last difficulty change.
ACKs for top commit:
jlopp:
tACK 376dc2cfb3
achow101:
ACK 376dc2cfb32806a8aa450589effe4d384e648398
ismaelsadeeq:
Tested ACK 376dc2cfb32806a8aa450589effe4d384e648398
pablomartin4btc:
tACK 376dc2cfb32806a8aa450589effe4d384e648398
Tree-SHA512: 02d52f622e9cb7a1240c5d124510dd75d03f696f119b2625b0befd60b004ec50ff1a2d5515e0e227601adeecd837e0778ed131ee2a8c5f75f1b824be711213a7
37d150d8c5ffcb2bddcd99951a739e97571194c7 refactor: Add more negative `!m_banned_mutex` thread safety annotations (Hennadii Stepanov)
0fb29087080a4e60d7c709ff5edf14e830ef3a69 refactor: replace RecursiveMutex m_banned_mutex with Mutex (w0xlt)
784c316f9cb664c9577cbfed1873bae573efd1b4 scripted-diff: rename m_cs_banned -> m_banned_mutex (w0xlt)
46709c5f27bf6cbc8eba1298b04bd079da2cdded refactor: Get rid of `BanMan::SetBannedSetDirty()` (Hennadii Stepanov)
d88c0d8440cf640ef4f2c7a40b8b8b31bfd38f23 refactor: Get rid of `BanMan::BannedSetIsDirty()` (Hennadii Stepanov)
Pull request description:
This PR is an alternative to bitcoin/bitcoin#24092. Last two commit have been cherry-picked from the latter.
ACKs for top commit:
maflcko:
ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7 🎾
achow101:
ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7
theStack:
Code-review ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7
vasild:
ACK 37d150d8c5ffcb2bddcd99951a739e97571194c7
Tree-SHA512: 5e9d40101a09af6e0645a6ede67432ea68631a1b960f9e6af0ad07415ca7718a30fcc1aad5182d1d5265dc54c26aba2008fc9973840255c09adbab8fedf10075
Clarify that supported versions of GCC are not affected, and that Clang
prior to version 15 still requires the explicit -latomic linking, when
compiling for 32-bit.
a5e39d325da4eeb9273fb7c919fcbfbc721ed75d Fee estimation: extend bucket ranges consistently (Anthony Towns)
Pull request description:
When calculating a median fee for a confirmation target at a particular threshold, we analyse buckets in ranges rather than individually in case some buckets have very little data. This patch ensures the breaks between ranges are independent of the the confirmation target.
Fixes#20725
ACKs for top commit:
ismaelsadeeq:
Code review ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d
glozow:
btw what I meant by [this](https://github.com/bitcoin/bitcoin/pull/21161#pullrequestreview-1350258467) was ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d
jonatack:
Initial ACK a5e39d325da4eeb9273fb7c919fcbfbc721ed75d
Tree-SHA512: 0edf4e56717c4ab8d4ab0bc0f1d7ab36a13b99de12f689e55c9142c6b81691367ffd8df2e8260c5e14335310b1a51770c6c22995db31109976239befcb558ef8
9b3da70bd06b45482e7211aa95637a72bd115553 [test] DisconnectedBlockTransactions::DynamicMemoryUsage (glozow)
b2d04479647af64ad7cf5ebfb6175251b2f6b72e bugfix: correct DisconnectedBlockTransactions memory usage (stickies-v)
f4254e209801d6a790b5f0c251c0b32154a4e3cc assume duplicate transactions are not added to `iters_by_txid` (ismaelsadeeq)
29eb219c1247993378fce06c8f71aab20736c237 move only: move implementation code to disconnected_transactions.cpp (ismaelsadeeq)
81dfeddea70ae5feeaf79062585c2ff9f33c0ca3 refactor: update `MAX_DISCONNECTED_TX_POOL` from kb to bytes (ismaelsadeeq)
Pull request description:
This PR is a follow-up to fix review comments and a bugfix from #28385
The PR
- Updated `DisconnectedBlockTransactions`'s `MAX_DISCONNECTED_TX_POOL` from kb to bytes.
- Moved `DisconnectedBlockTransactions` implementation code to `kernel/disconnected_transactions.cpp`.
- `AddTransactionsFromBlock` now assume duplicate transactions are not passed by asserting after inserting each transaction to `iters_by_txid`.
- Included a Bug fix: In the current master we are underestimating the memory usage of `DisconnectedBlockTransactions`.
* When adding and subtracting `cachedInnerUsage` we call `RecursiveDynamicUsage` with `CTransaction` which invokes this [`RecursiveDynamicUsage(const CTransaction& tx)`](6e721c923c/src/core_memusage.h (L32)) version of `RecursiveDynamicUsage`, the output of that call only account for the memory usage of the inputs and outputs of the `CTransaction`, this omits the memory usage of the `CTransaction` object and the control block.
* This PR fixes this bug by calling `RecursiveDynamicUsage` with `CTransactionRef` when adding and subtracting `cachedInnerUsage` which invokes [`RecursiveDynamicUsage(const std::shared_ptr<X>& p)`](6e721c923c/src/core_memusage.h (L67)) version of `RecursiveDynamicUsage` the output of the calculation accounts for the` CTransaction` object, the control blocks, inputs and outputs memory usage.
* see [comment ](https://github.com/bitcoin/bitcoin/pull/28385#discussion_r1322948452)
- Added test for DisconnectedBlockTransactions memory limit.
ACKs for top commit:
stickies-v:
ACK 9b3da70bd06b45482e7211aa95637a72bd115553 - nice work!
BrandonOdiwuor:
re ACK 9b3da70bd06b45482e7211aa95637a72bd115553
glozow:
ACK 9b3da70bd06b45482e7211aa95637a72bd115553
Tree-SHA512: 69b9595d09f4d0209038f97081d790cea92ccf63efb94e9e372749979fcbe527f7f17a8e454720cedd12021be0c8e11cf99874625d3dafd9ec602b12dbeb4098
8047bb6feaa9ee5d6c1edb7640baaf228450bc6b build: Update `qt` package up to 5.15.11 (Hennadii Stepanov)
Pull request description:
In the light of https://github.com/bitcoin/bitcoin/pull/28622, we probably have to patch Qt. It seems reasonable to update it up to the latest available version before doing that.
ACKs for top commit:
TheCharlatan:
ACK 8047bb6feaa9ee5d6c1edb7640baaf228450bc6b
Tree-SHA512: b4d7df2ff059b8f58c3202d913237c0d39a962748658f1ce853884dca095fbda5f56d4d68f73a1bc8da2f295e96a20927306e148b41a9f4afc42c8edb11c3729
`lcov`'s `-a` option takes an argument. With `LCOV_OPTS` immediately
after `-a`, the first additional argument becomes the argument to `-a`
which is incorrect.
Also add `LCOV_OPTS` to more `lcov` calls.
