When the detailed peers list is requested, return the shortened services in the
-netinfo header in the same format as the "serv" column, instead of the full names
list in the report.
e017ef3c7eb775e2cf999674df341be56f7ba72d init: make `-blockmaxweight` startup option debug-only (ismaelsadeeq)
Pull request description:
This PR updates `-blockmaxweight` startup option to be debug-only so that it will be hidden from help text.
The option is currently unlikely to be used on mainnet, after the addition of the new `blockreservedweight` option. however it can be useful for test and signet network see https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2925674473
ACKs for top commit:
Sjors:
tACK e017ef3c7eb775e2cf999674df341be56f7ba72d
fjahr:
ACK e017ef3c7eb775e2cf999674df341be56f7ba72d
polespinasa:
tACK e017ef3c7eb775e2cf999674df341be56f7ba72d
Tree-SHA512: 6c18781826b2f96b13b70b7f1624481f5971746a613079d0d9528366f274ba657a02611f134d7a64f35ecb7e5faf2e3cd025458b04574ac68f804372f6eb715f
a3cf623364e84819bc16fd407b80d8dba46bbcb5 test: Test max_selection_weight edge cases (Murch)
57fe8acc8a8471ec41ac5f3c3764458431880e4e test: Check max_weight_exceeded error (Murch)
Pull request description:
I tested all of the reported surviving mutants that @brunoerg reported in https://gist.github.com/brunoerg/834063398d5002f738506d741513e310.
I found that all Mutants except for 12, 14, 17, 37, and 39 were now being caught by one of the existing tests. This fixes Mutants 14, 37, and 39.
Mutant 17 is not fixed, because I consider it acceptable that running BnB for 100,001 instead of 100,000 comparisons doesn’t cause an issue, and Mutant 12 is not yet fixed, because at `fee` = `long_term_fee`, the waste of inputs is 0 and only excess matters, and I haven’t evaluated yet, whether it needs to be fixed.
ACKs for top commit:
achow101:
ACK a3cf623364e84819bc16fd407b80d8dba46bbcb5
jlest01:
ACK a3cf623364
brunoerg:
code review ACK a3cf623364e84819bc16fd407b80d8dba46bbcb5
Tree-SHA512: db67c52127ed98f809f64a903c6b3a012e56cf665a0cd851457af7c85c37ec3af8bb72035d7ad370dd883f99cf3014464e3576559899e37c1d6ee01230511754
cc33e4578946c68d6d333f35c48f8cbf3f75f6cf test: improve assertion for SRD max weight test (yancy)
Pull request description:
Replace generic assertion with a result specific assertion showing the correctness of the solution found. If the max weight parameter is exceeded, the least valuable `UTXOs` are removed from the result. Therefore, only the most valued _encountered_ `UTXO's` are selected. While the smallest set would include all the most valued `UTXO's`, in the case of the test there is one high value `UTXO` that is never found before the target value is reached.
Correct the test comment to be more specific about why the assertion is a good result.
ACKs for top commit:
murchandamus:
ACK cc33e4578946c68d6d333f35c48f8cbf3f75f6cf
furszy:
ACK cc33e4578946c68d6d333f35c48f8cbf3f75f6cf
Tree-SHA512: bad224063ba830c27fba1b7b80e411ac7cd6c3edcb60bade4e6e3010f3b5d360a921de742c7c20efea8fa839d7939f338270658f66bbcebedebe5c5c8a3e8f9b
c0642e558a02319ade33dc1014e7ae981663ea46 [fuzz] fix latency score check in txorphan_protected (glozow)
3d4d4f0d92d42809e74377e4380abdc70f74de5d scripted-diff: rename "ann" variables to "latency_score" (monlovesmango)
3b924489238220710326e9031c7aaa0d606c9064 [doc] comment fixups for orphanage changes (glozow)
1384dbaf6d0bfcdb05f97e1e3cb3d5e498bee505 [config] emit warning for -maxorphantx, but allow it to be set (glozow)
b10c55b298d4d2b7dddfecdbeb0edc624b8e6eb2 fix up TxOrphanage lower_bound sanity checks (glozow)
cfd71c67043a2a46950fd3f055afbe4a93922f75 scripted-diff: rename TxOrphanage outpoints index (glozow)
edb97bb3f151600f00c94a2732d2595446011295 [logging] add logs for inner loop of LimitOrphans (glozow)
8a58d0e87d70580ae47da228e2f88cd53c40c675 scripted-diff: rename OrphanTxBase to OrphanInfo (glozow)
cc50f2f0df6e6e2cc9b9aeb3c3c8e1c78fa5be1d [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans (glozow)
ed24e016969098c486f413f4f57dcffe35241785 [optimization] Maintain at most 1 reconsiderable announcement per wtxid (Pieter Wuille)
af7402ccfa7f19177b5f422f596a3ab2bd1e9633 [refactor] make TxOrphanage keep itself trimmed (glozow)
d1fac25ff3c3ac090b68e370efc6dd9374b6ad3b [doc] 31829 release note (glozow)
Pull request description:
Followup to #31829:
- Release notes
- Have the orphanage auto-trim itself whenever necessary (and test changes) https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169508690
- Reduce duplicate reconsiderations by keeping track of which txns are already reconsiderable so we only mark it for reconsideration for 1 peer at a time https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3001627814
- Rename `OrphanTxBase` to `OrphanInfo`
- Get rid of `Size()` method by replacing all calls with `CountUniqueOrphans`
- Rename outpoints index since they point to wtxids, not iterators https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205557613
- Add more logging in the `LimitOrphans` inner loop to make it easy to see which peers are being trimmed https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3074385460
ACKs for top commit:
sipa:
utACK c0642e558a02319ade33dc1014e7ae981663ea46
marcofleon:
Nice, ACK c0642e558a02319ade33dc1014e7ae981663ea46
Tree-SHA512: f298eae92cf906ed5e4f15a24eeffa7b9e620bcff457772cd77522dd9f0b3b183ffc976871b1b0e6fe93009e64877d518e53d4b9e186e0df58fc16d17f6de90a
444dcb2f9944ad5208bf00c9f197da7b2c98063c fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging` (Sebastian Falbesoner)
Pull request description:
In the `txgraph` fuzz test, the `CommitStaging` step updates the `SimTxGraph` levels simply by erasing the front (=main) one in the `sims` vector, i.e. the staging level instance takes the place of the main level instance:
83a2216f52/src/test/fuzz/txgraph.cpp (L668-L672)
This also includes the `real_is_optimal` flag (reflecting whether the corresponding real graph is known to be optimally linearized), without taking into account that this flag should only be set if _both_ levels before the commiting are optimal.
E.g. in case of #33097, at this point the main level is not optimally linearized, while the staging level is, and due to the incorrect propagation of the latter the simulation incorrectly assumes that the main level is optimal after, leading to the assertion fail in the additional checks that are ran in this case[1]. Fix this by setting the flag in the resulting main level explicitly. This is done in a generic way, in case there will ever be more than two levels (not sure what is planned in this direction), a simpler alternative would be e.g. `main_optimal = sim[0].real_is_optimal && sim[1].real_is_optimal`.
Fixes#33097.
[1] see 0aedf09ccc for the printf-debug-session-clutter, if that is useful/interesting for anyone (most of the output turned out to be irrelevant to the actual cause of #33097, but it was an entertaining way to discover the interface and get a first glimpse of `TxGraph` internals as a cluster-mempool newbie).
ACKs for top commit:
sipa:
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
glozow:
ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
Tree-SHA512: c20580e14628fcdc34dabb646a097e02e95b26c5740fcd5ce50f3472e4ee08f20b9a146c9ff16c85e19e57b05af1560e41a9220289c60c15083ad897dc62a0f0
In the `txgraph` fuzz test, the `CommitStaging` step updates the
`SimTxGraph` levels simply by erasing the front (=main) one in the
`sims` vector, i.e. the staging level instance takes the place of the
main level instance. This also includes the `real_is_optimal` flag
(reflecting whether the corresponding real graph is known to be
optimally linearized), without taking into account that this flag
should only be set if _both_ levels before the commiting are optimal.
E.g. in case of #33097, the main level is not optimally linearized,
while the staging level is, and due to the incorrect propagation of the
latter to the simulation incorrectly assumes that the main level is
optimal, leading to the assertion fail. Fix this by setting the flag
in the resulting main level explicitly.
Resolves the fuzzing assertion fail in issue #33097.
e07e2532b4d7b4a549722e56cb2913a6081ccbaf test: fix anti-fee-sniping off-by-one error (ishaanam)
Pull request description:
This fixes the off-by-one error in the anti-fee-sniping tests for `send` and `sendall`. `assert_greater_than` fails if the two values are equal.
Closes#33114
ACKs for top commit:
achow101:
ACK e07e2532b4d7b4a549722e56cb2913a6081ccbaf
glozow:
utACK e07e2532b4d7b4a549722e56cb2913a6081ccbaf
Tree-SHA512: 6c9c3d1256faf563361946703d9a51279777d73bc1a849873e03e5b5db52c3c2b9dea4bfe27b1f01b9c830ca246200a895b6a28484da6d822b93b0c7cba237c1
-BEGIN VERIFY SCRIPT-
sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.h
sed -i 's/max_global_ann/max_global_latency_score/g' src/test/orphanage_tests.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/test/fuzz/txorphan.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/bench/txorphanage.cpp
sed -i 's/max_ann/max_lat/g' src/node/txorphanage.cpp
-END VERIFY SCRIPT-
This introduces an invariant that TxOrphanageImpl never holds more than one
announcement with m_reconsider=true for a given wtxid. This avoids duplicate
work, both in the caller might otherwise reconsider the same transaction multiple
times before it is ready, and internally in AddChildrenToWorkSet, which might
otherwise iterate over all announcements multiple times.
eb65f57f319dc4e2ea8c83cf7e283c36f1c0d53b [test] setmocktime instead of waiting in 1p1c tests (glozow)
70772dd4693ba6b09901a6b812eef21fa240b666 [test] cut the number of transactions involved in 1p1c DoS tests (glozow)
Pull request description:
It was brought to my attention that the runtime of this test is Too Damn High. The test is slow due to the many `wait_for_getdata`s with delays (inbound peer + txid request) and the large volume of messages sent in the dos-related tests. This PR cuts the runtime by about 60% by reducing the number of messages/transactions and using `setmocktime` instead of waiting.
On my machine, master:
```
84.51s user 1.55s system 57% cpu 2:28.53 total
```
After first commit (about 1min faster):
```
28.29s user 0.88s system 35% cpu 1:22.84 total
```
After second commit (about 30sec faster):
```
28.17s user 0.87s system 59% cpu 49.082 total
```
Reviewers should verify that the transactions in the DoS tests are still enough to cause evictions, and that the `bumpmocktime` amounts are not more than necessary.
Alternatives:
- If we don't like mocking the times, we can use outbound connections for all the peers. However, that approach won't improve the runtime as much because we impose a 2-second delay on all txid requests regardless of peer type.
- Note that `noban_tx_relay` is not relevant for this test because all delays are related to downloading, not announcing.
ACKs for top commit:
achow101:
ACK eb65f57f319dc4e2ea8c83cf7e283c36f1c0d53b
w0xlt:
ACK eb65f57f31
Tree-SHA512: 6ffe1f9e5144653e2ded744cec9ddb62ad728c587705542565400a0e8f1fba4843aced4e0d929843874ca7f56f670f5871b7e009ff6be58b791ab24d2e6fcc0e
ea17a9423fb431a86d36927b02d3624f654fd867 [doc] release note for relaxing requirement of all unconfirmed parents present (glozow)
12f48d5ed302e92a334dbe971c66df467d402655 test: add chained 1p1c propagation test (Greg Sanders)
525be56741cff5f89d596b8a0c44df00f2209bcb [unit test] package submission 2p1c with 1 parent missing (glozow)
f24771af0581e8117bd638227469e37cc69c5103 relax child-with-unconfirmed-parents rule (glozow)
Pull request description:
Broadens the package validation interface, see #27463 for wider context.
On master, package rules include that (1) the package topology must be child-wth-parents (2) all of the child's unconfirmed parents must be present. This PR relaxes the second rule, leaving the first rule untouched (there are plans to change that as well, but not here).
Original motivation for this rule was based on the idea that we would have a child-with-unconfirmed-parents package relay protocol, and this would verify that the peer provided the "correct" package. For various reasons, we're not planning on doing this. We could potentially do this for ancestor packages (with a similar definition that all UTXOs to make the tx valid are available in this package), but it's also questionable whether it's useful to enforce this.
This rule gets in the way of certain usage of 1p1c package relay currently. If a transaction has multiple parents, of which only 1 requires a package CPFP, this rule blocks the package from relaying. Even if all the non-low-feerate parents are already in mempool, when the p2p logic submits the 1p1c package, it gets rejected for not meeting this rule.
ACKs for top commit:
ishaanam:
re-utACK ea17a9423fb431a86d36927b02d3624f654fd867
instagibbs:
ACK ea17a9423fb431a86d36927b02d3624f654fd867
Tree-SHA512: c2231761ae7b2acea10a96735e7a36c646f517964d0acb59bacbae1c5a1950e0223458b84c6d5ce008f0c1d53c1605df0fb3cd0064ee535ead006eb7c0fa625b
c1574381168573c561ebddf1945d2debefb340f7 qa: test that we do disconnect upon a second invalid compact block being announced (Antoine Poinsot)
fb2dcbb160bd7d61d53d8150df7f2aaabc32f78c qa: test cached failure for compact block (Antoine Poinsot)
f12d8b104e0e6b7f3a01036c8abee8444485e013 qa: test a compact block with an invalid transaction (Antoine Poinsot)
d6c37b28a7820fd48ec61e959becd9269d144628 qa: remove unnecessary tx removal from compact block (Antoine Poinsot)
Pull request description:
In thinking about https://github.com/bitcoin/bitcoin/pull/33050 and https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3111631541, i went through the code paths for peer disconnection upon submitting an invalid block. It turns out that the fact we exempt a peer from disconnection upon submitting an invalid compact block was not properly tested, as can be checked with these diffs:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44c..d243fb88d4 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1805,10 +1805,10 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
// The node is providing invalid data:
case BlockValidationResult::BLOCK_CONSENSUS:
case BlockValidationResult::BLOCK_MUTATED:
- if (!via_compact_block) {
+ //if (!via_compact_block) {
if (peer) Misbehaving(*peer, message);
return;
- }
+ //}
break;
case BlockValidationResult::BLOCK_CACHED_INVALID:
{
```
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..e8e0c805367 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1814,10 +1814,10 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
{
// Discourage outbound (but not inbound) peers if on an invalid chain.
// Exempt HB compact block peers. Manual connections are always protected from discouragement.
- if (peer && !via_compact_block && !peer->m_is_inbound) {
+ //if (peer && !via_compact_block && !peer->m_is_inbound) {
if (peer) Misbehaving(*peer, message);
return;
- }
+ //}
break;
}
case BlockValidationResult::BLOCK_INVALID_HEADER:
```
We do have a test for this, but it actually uses a coinbase witness commitment error, which is checked much earlier in `FillBlock`. This PR adds coverage for the two exemptions in `MaybePunishNodeForBlock`.
ACKs for top commit:
kevkevinpal:
ACK [c157438](c157438116)
nervana21:
tACK [c157438](c157438116)
instagibbs:
crACK c1574381168573c561ebddf1945d2debefb340f7
stratospher:
ACK c1574381168573c561ebddf1945d2debefb340f7.
Tree-SHA512: a77d5a9768c0d73f122b06db2e416e80d0b3c3fd261dae8e340ecec2ae92d947d31988894bc732cb6dad2e338b3c82f33e75eb3280f8b0933b285657cf3b212c
4b80147feb97300e92e1f940b8d989a0af331e06 test: Perform backup filename checks in migrate_and_get_rpc (Ava Chow)
Pull request description:
Some test cases were unnecessarily checking the backup filename, which involved setting the mocktime before `migrate_and_get_rpc`. However, this could cause a failure if the test was slow since `migrate_and_get_rpc` also sets the mocktime. Since it also already checks that the backup file is named correctly, there's no need for those tests to also do their own mocktime and filename check.
The CI failure can be reproduced locally by adding a sleep to `migrate_and_get_rpc`:
```diff
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 704204425c7..e87a6100623 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -129,6 +129,7 @@ class WalletMigrationTest(BitcoinTestFramework):
assert_equal(w["warnings"], ["This wallet is a legacy wallet and will need to be migrated with migratewallet before it can be loaded"])
# Mock time so that we can check the backup filename.
+ time.sleep(1)
mocked_time = int(time.time())
self.master_node.setmocktime(mocked_time)
# Migrate, checking that rescan does not occur
```
Fixes#33096
ACKs for top commit:
fjahr:
reACK 4b80147feb97300e92e1f940b8d989a0af331e06
Sammie05:
tACK 4b80147
pablomartin4btc:
utACK 4b80147feb97300e92e1f940b8d989a0af331e06
rkrux:
ACK 4b80147feb97300e92e1f940b8d989a0af331e06
Tree-SHA512: 045d4acf2ad0b56a7083ff2ee5ef09f0d74ad097c01a290660daca096c71fc07109848024256d84f74abbc87dd52691d160f9968b3654726626d3dbd21a84ab6
Some test cases were unnecessarily checking the backup filename, which
involved setting the mocktime before `migrate_and_get_rpc`. However,
this could cause a failure if the test was slow since
`migrate_and_get_rpc` also sets the mocktime. Since it also already
checks that the backup file is named correctly, there's no need for
those tests to also do their own mocktime and filename check.
7aa5b67132dfb71e915675a3dbcb806284e08197 ci: remove DEBUG_LOCKORDER from TSAN job (fanquake)
b09af2ce508185086bb551bfeb1409355c897e7b ci: instrument libc++ in TSAN job (fanquake)
6653cafd0b70b0e7a29c6cfe236d3bf9d1bce91e ci: allow libc++ instrumentation other than msan (fanquake)
Pull request description:
Allow for instrumenting libc++ with a sanitizer other than MemoryWithOrigins.
Would also close#33087, as with the extra instrumentation, the issue from https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3114706601 is avoided (also see https://github.com/bitcoin/bitcoin/pull/33081), and we can drop `DEBUG_LOCKORDER`.
ACKs for top commit:
maflcko:
re-ACK 7aa5b67132dfb71e915675a3dbcb806284e08197 🦀
dergoegge:
utACK 7aa5b67132dfb71e915675a3dbcb806284e08197
Tree-SHA512: 95f123e37da5e81d571244e4b1cd7658107676f1ea763ff16e5b69f4dfadb88236a577bb2ee52230ff542872c1da151c88fc50aba0f32540e765080120cec55e
9954d6c833381a44e1ea34d182ffe4d61b65d2ba depends: hard-code necessary c(xx)flags rather than setting them per-host (Cory Fields)
Pull request description:
The per-host flag variables hold platform-specific defaults that are ignored when flag environment variables are set, so it was wrong for them to contain -std options relied on by package definitions.
Additionally, these flags (-pipe and -std=xx) will no longer be passed into the CMake build, meaning less duplication in the build summary.
Pulled out of #31920.
ACKs for top commit:
achow101:
ACK 9954d6c833381a44e1ea34d182ffe4d61b65d2ba
ryanofsky:
Code review ACK 9954d6c833381a44e1ea34d182ffe4d61b65d2ba. No changes since last review other than improving the commit message. Change overall makes sense because it deduplicates host definitions, stops dropping `-std` flags from package builds when custom CFLAGS/CXXFLAGS environment variables are set, and stops passing duplicate flags to cmake that have no effect.
theuni:
ACK 9954d6c833381a44e1ea34d182ffe4d61b65d2ba
Tree-SHA512: 62a2a21b741893cf708ca71fb5f0626c30d52685c845f9016f428a5e38fc8515acd4bf2c83635d6505b63830d1c296472026ec3d341c8069f1e490a991b6b5ef
fac90e5261b811739ada56e06ea793a12f9c2c3d test: Check that the GUI interactive reindex works (MarcoFalke)
faaaddaaf8e5a63f19c4fc66aa79134987775f96 init: [gui] Avoid UB/crash in InitAndLoadChainstate (MarcoFalke)
Pull request description:
`InitAndLoadChainstate` is problematic, when called twice in the GUI. This can happen when it returns a failure and the user selects an interactive reindex.
There are several bugs that have been introduced since the last time this was working correctly:
* The first one is a crash (assertion failure), which happens due to a cached tip block in the notifiications from the previous run. See https://github.com/bitcoin/bitcoin/pull/31346#discussion_r2207914726
* The second one is UB (use-after-free), which happens because the block index db in the blockmanager is not reset. See https://github.com/bitcoin/bitcoin/pull/30965#discussion_r2207822121
Fix both bugs by resetting any dirty state in `InitAndLoadChainstate`.
Also, add a test, because I don't really want to keep testing this manually every time. (A failing test run can be seen in https://github.com/bitcoin/bitcoin/pull/32979/checks)
ACKs for top commit:
achow101:
ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
TheCharlatan:
ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
mzumsande:
Tested ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
Tree-SHA512: 9f744d36e7cdd3f5871764386ec5a5cca1ae144f1bacc26c07e60313c2bdacdc5fca351aa185cb51359540eea4534dda17e4fb6073ad90f91ba0a6936faeead8
3a03f075606b19e411b8bd19870242e0e0b58fcb qt: Avoid header circular dependency (Anthony Towns)
25884bd89684262b94e09c9904c1560d5ba87d86 qt, refactor: Move `FreespaceChecker` class into its own module (Hennadii Stepanov)
Pull request description:
For some reason, the MOC compiler in older versions of Qt 6 fails to parse `qt/intro.cpp`, as noted in [this comment](https://github.com/bitcoin/bitcoin/pull/32998#issuecomment-3082011233).
This PR proposes a move-only refactoring to simplify the source structure by eliminating the need for the inline `#include <qt/intro.moc>`, thereby effectively working around the issue.
Required for https://github.com/bitcoin/bitcoin/pull/32998.
ACKs for top commit:
ajtowns:
ACK 3a03f075606b19e411b8bd19870242e0e0b58fcb
Tree-SHA512: 4a7261f04fff9bd8edd4dc2df619c90e06417e19da672dd688a917cd0b9a324a6db7185a47c48f0385713b5e6c45d2204bef58cbe6c77299386136ed5682bd8d
c6e2c31c55123cc97b4400bcbf3c37a39b067a22 rpc: unhide waitfor{block,newblock,blockheight} (Sjors Provoost)
0786b7509acd7e160345eea5fc25acd3c795d01c rpc: add optional blockhash to waitfornewblock (Sjors Provoost)
Pull request description:
The `waitfornewblock` is inherently racy as the tip may have changed since the last RPC call, and can even change during initial processing of this call.
Add an optional `blockhash` argument so the caller can specify their current tip. Return immediately if our tip is different.
I've made it fail if `LookupBlockIndex` fails. This should never happen if the user got the block hash from our RPC in the first place.
Finally, the `waitfor{block,newblock,blockheight}` RPC methods are no longer hidden in `help`:
- the changes in #30409 ensured these methods _could_ work in the GUI
- #31785 removed the guards that prevented GUI users from using them
- this PR makes `waitfornewblock` reliable
So there's no more reason to hide them.
ACKs for top commit:
TheCharlatan:
Re-ACK c6e2c31c55123cc97b4400bcbf3c37a39b067a22
ryanofsky:
Code review ACK c6e2c31c55123cc97b4400bcbf3c37a39b067a22. Just rebased and tweaked documentation since last review.
glozow:
utACK c6e2c31c55123cc97b4400bcbf3c37a39b067a22
Tree-SHA512: 84a0c94cb9a2e4449e7a395cf3dce1650626bd852e30e0e238a1aafae19d57bf440bfac226fd4da44eaa8d1b2fa4a8c1177b6c716235ab862a72ff5bf8fc67ac
cab6736b701f203d6e823e1b5d619368d8d4c5e0 ci: remove ninja-build from MSAN jobs (fanquake)
Pull request description:
It is part of `CI_BASE_PACKAGES`.
ACKs for top commit:
maflcko:
review ACK cab6736b701f203d6e823e1b5d619368d8d4c5e0 🕸
hebasto:
ACK cab6736b701f203d6e823e1b5d619368d8d4c5e0, I have reviewed the code and it looks OK.
Tree-SHA512: 8da5f0b07310a1e003d405ade19408b390781121a317ecc0ebdf48cb63bb3abf39bcfb635e4a43200568e0debabb463c2a3a843705e625fa455609eb3f0ba416
c2ed576d2caf282a51aa9df5df55d844ab1da794 fuzz: cover BanMan::IsDiscouraged (brunoerg)
Pull request description:
This PR adds fuzz coverage for the `IsDiscouraged` function in the banman target. This is the only function missing from `BanMan`.
ACKs for top commit:
maflcko:
lgtm ACK c2ed576d2caf282a51aa9df5df55d844ab1da794
marcofleon:
ACK c2ed576d2caf282a51aa9df5df55d844ab1da794
Tree-SHA512: 1dc5fc138f89413c46ed41195940f4c578ef996ce84595271b7433cae8a8f576205b649b493a7ec4804c712327d6c77b1004ba116b0144916377042adaaf6c5f
5888b4a2a5566c64141b78a0e7660a166ec99775 doc: add note for watch-only wallet migration (rkrux)
Pull request description:
This was suggested in a previous PR #31423.
ACKs for top commit:
achow101:
ACK 5888b4a2a5566c64141b78a0e7660a166ec99775
brunoerg:
reACK 5888b4a2a5566c64141b78a0e7660a166ec99775
jonatack:
ACK 5888b4a2a5566c64141b78a0e7660a166ec99775
Tree-SHA512: 96e51eda30a1f31cfd82ae3296ca97c9236599b18e19086dbde3a908f6fe66af8f2de7aa147bdb9ebccb2059c809a25ddfb0c23da57e1a84a35b62ca0a44e3c3
76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77 test: Migration of a wallet ending in `../` (David Gumberg)
f0bb3d50fef08d9f981eac45841ef2df7444031b test: Migration of a wallet ending in `/` (David Gumberg)
41faef5f80d69ed984af685bd9d2a43268009fb6 test: Migration fail recovery w/ `../` in path (David Gumberg)
63c6d364376907c10b9baa3c6f4d72e3f1881abc test: Migration of a wallet with `../` in path. (David Gumberg)
70f1c99c901de64d6ccea793b7e267e20dfa49cf wallet: Fix migration of wallets with pathnames. (David Gumberg)
f6ee59b6e2995a3916fb4f0d4cbe15ece2054494 wallet: migration: Make backup in walletdir (David Gumberg)
e22c3599c6772730e72e17fc68c99feea09b4d29 test: wallet: Check direct file backup name. (David Gumberg)
Pull request description:
Support for wallets outside of the default wallet directory was added in #11687, and these external wallets can be specified with paths relative to the wallet directory, e.g. `bitcoin-cli loadwallet ../../mywallet`. In the RPC commands, there is no distinction between a wallet's 'name' and a wallet's 'path'. This PR fixes an issue with wallet backup during migration where the wallet's 'name-path' is used in the backup filename. This goes south when that filename is appended to the directory where we want to put the file and the wallet's 'name' actually gets treated as a path:
```cpp
fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime()));
fs::path backup_path = this_wallet_dir / backup_filename;
```
Attempting to migrate a wallet with the 'name' `../../../mywallet` results in a backup being placed in `datadir/wallets/../../../mywallet/../../../mywallet_1744683963.legacy.bak`.
If permissions don't exist to write to that folder, migration can fail.
The solution implemented here is to put backup files in the top-level of the node's `walletdir` directory, using the folder name (and in some rare cases the file name) of the wallet to name the backup file:
9fa5480fc4/src/wallet/wallet.cpp (L4254-L4268)
##### Steps to reproduce on master
Build and run `bitcoind` with legacy wallet creation enabled:
```bash
$ cmake -B build -DWITH_BDB=ON && cmake --build build -j $(nproc)
$ ./build/bin/bitcoind -regtest -deprecatedrpc=create_bdb
```
Create a wallet with some relative path specifiers (exercise caution with where this file may be written)
```bash
$ ./build/bin/bitcoin-cli -regtest -named createwallet wallet_name="../../../myrelativewallet" descriptors=false
```
Try to migrate the wallet:
```bash
$ ./build/bin/bitcoin-cli -regtest -named migratewallet wallet_name="../../../myrelativewallet"
```
You will see a message in `debug.log` about trying to backup a file somewhere like: `/home/user/.bitcoin/regtest/wallets/../../../myrelativewallet/../../../myrelativewallet_1744686627.legacy.bak` and migration might fail because `bitcoind` doesn't have permissions to write the backup file.
ACKs for top commit:
pablomartin4btc:
tACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77
achow101:
ACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77
ryanofsky:
Code review ACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77. Nice changes that (1) fix potential errors when names of wallets being migrated contain slashes, and (2) store migration backups in the top-level `-walletdir` instead of in individual wallet subdirectories.
Tree-SHA512: 5cf6ed9f44ac7d204e4e9854edd3fb9b43812e930f76343b142b3c19df3de2ae5ca1548d4a8d26226d537bca231e3a50b3ff0d963c200303fb761f2b4eb3f0d9
fa45ccc15dfc52e798da62548dc43d1bd7889c9a doc: Add legacy wallet removal release notes (MarcoFalke)
Pull request description:
This spans over several pulls, so add a single note for all of them.
ACKs for top commit:
glozow:
lgtm ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
achow101:
ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
pablomartin4btc:
ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
janb84:
re ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
Tree-SHA512: e753cc3afbd66a88099ff62c2591aa31d32d784098e433e392c20a8dfd40d5c85807e955b264a287c3778d68605cd7022278886a43cd1635c080d563c88fc0cc
aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f test: test sendall and send do anti-fee-sniping (ishaanam)
20802c7b65f4b196a3ed0aca876974153db55d9d wallet, rpc: add anti-fee-sniping to `send` and `sendall` (ishaanam)
Pull request description:
Currently, `send` and `sendall` don't do anti-fee-sniping because they don't use `CreateTransaction`. This PR adds anti-fee-sniping to these RPCs by calling `DiscourageFeeSniping` from `FinishTransaction` when the user does not specify a locktime.
ACKs for top commit:
achow101:
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
murchandamus:
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
glozow:
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
Tree-SHA512: d4f1b43b5bda489bdba46b0af60e50bff0de604a35670e6ea6e1de2b539f16b3f68805492f51d6d2078d421b63432ca22a561a5721d1a37686f2e48284e1e646
