86d7135e36 [p2p] only attempt 1p1c when both txns provided by the same peer (glozow)
f7658d9b14 [cleanup] remove p2p_inv from AddTxAnnouncement (glozow)
063c1324c1 [functional test] getorphantxs reflects multiple announcers (glozow)
0da693f7e1 [functional test] orphan handling with multiple announcers (glozow)
b6ea4a9afe [p2p] try multiple peers for orphan resolution (glozow)
1d2e1d709c [refactor] move creation of unique_parents to helper function (glozow)
c6893b0f0b [txdownload] remove unique_parents that we already have (glozow)
163aaf285a [fuzz] orphanage multiple announcer functions (glozow)
22b023b09d [unit test] multiple orphan announcers (glozow)
96c1a822a2 [unit test] TxOrphanage EraseForBlock (glozow)
04448ce32a [txorphanage] add GetTx so that orphan vin can be read (glozow)
e810842acd [txorphanage] support multiple announcers (glozow)
62a9ff1870 [refactor] change type of unique_parents to Txid (glozow)
6951ddcefd [txrequest] GetCandidatePeers (glozow)
Pull request description:
Part of #27463.
(Transaction) **orphan resolution** is a process that kicks off when we are missing UTXOs to validate an unconfirmed transaction. We currently request missing parents by txid; BIP 331 also defines a way to [explicitly request ancestors](https://github.com/bitcoin/bips/blob/master/bip-0331.mediawiki#handle-orphans-better).
Currently, when we find that a transaction is an orphan, we only try to resolve it with the peer who provided the `tx`. If this doesn't work out (e.g. they send a `notfound` or don't respond), we do not try again. We actually can't, because we've already forgotten who else could resolve this orphan (i.e. all the other peers who announced the transaction).
What is wrong with this? It makes transaction download less reliable, particularly for 1p1c packages which must go through orphan resolution in order to be downloaded.
Can we fix this with BIP 331 / is this "duct tape" before the real solution?
BIP 331 (receiver-initiated ancestor package relay) is also based on the idea that there is an orphan that needs resolution, but it's just a new way of communicating information. It's not inherently more honest; you can request ancestor package information and get a `notfound`. So ancestor package relay still requires some kind of procedure for retrying when an orphan resolution attempt fails. See the #27742 implementation which builds on this orphan resolution tracker to keep track of what packages to download (it just isn't rebased on this exact branch). The difference when using BIP 331 is that we request `ancpkginfo` and then `pkgtxns` instead of the parent txids.
Zooming out, we'd like orphan handling to be:
- Bandwidth-efficient: don't have too many requests out at once. As already implemented today, transaction requests for orphan parents and regular download both go through the `TxRequestTracker` so that we don't have duplicate requests out.
- Not vulnerable to censorship: don't give up too easily, use all candidate peers. See e.g. https://bitcoincore.org/en/2024/07/03/disclose_already_asked_for/
- Load-balance between peers: don't overload peers; use all peers available. This is also useful for when we introduce per-peer orphan protection, since each peer will have limited slots.
The approach taken in this PR is to think of each peer who announces an orphan as a potential "orphan resolution candidate." These candidates include:
- the peer who sent us the orphan tx
- any peers who announced the orphan prior to us downloading it
- any peers who subsequently announce the orphan after we have started trying to resolve it
For each orphan resolution candidate, we treat them as having "announced" all of the missing parents to us at the time of receipt of this orphan transaction (or at the time they announced the tx if they do so after we've already started tracking it as an orphan). We add the missing parents as entries to `m_txrequest`, incorporating the logic of typical txrequest processing, which means we prefer outbounds, try not to have duplicate requests in flight, don't overload peers, etc.
ACKs for top commit:
marcofleon:
Code review ACK 86d7135e36
instagibbs:
reACK 86d7135e36
dergoegge:
Code review ACK 86d7135e36
mzumsande:
ACK 86d7135e36
Tree-SHA512: 618d523b86e60c3ea039e88326d50db4e55e8e18309c6a20e8f2b10ed9e076f1de0315c335fd3b8abdabcc8b53cbceb66fb59147d05470ea25b83a2b4bd9c877
fa3efb5729 refactor: Introduce struct to hold a runtime format string (MarcoFalke)
fa6adb0134 lint: Remove unused and broken format string linter (MarcoFalke)
fadc6b9bac refactor: Check translatable format strings at compile-time (MarcoFalke)
fa1d5acb8d refactor: Use TranslateFn type consistently (MarcoFalke)
eeee6cf2ff refactor: Delay translation of _() literals (MarcoFalke)
Pull request description:
All translatable format strings are fixed. This change surfaces errors in them at compile-time.
The implementation achieves this by allowing to delay the translation (or `std::string` construction) that previously happened in `_()` by returning a new type from this function. The new type can be converted to `bilingual_str` where needed.
This can be tested by adding a format string error in an original string literal and observing a new compile-time failure.
Fixes https://github.com/bitcoin/bitcoin/issues/30530
ACKs for top commit:
stickies-v:
re-ACK fa3efb5729
ryanofsky:
Code review ACK fa3efb5729. Since last review added TranslateFn commit, clarified FormatStringCheck documentation, dropped redundant `inline` keyword
Tree-SHA512: 28fa1db11e85935d998031347bd519675d75c171c8323b0ed6cdd0b628c95250bb86b30876946cc48840ded541e95b8a152696f9f2b13a5f28f5673228ee0509
The linter has many implementation bugs and missing features.
Also, it is completely redundant with FormatStringCheck, which
constructs from ConstevalFormatString or a runtime format string.
`p2p_dns_seeds.py` would try to connect to the DNS server configured on
the machine and resolve `dummySeed.invalid`.
To block that configure an unavailable proxy which will be used also to
connect to the name server. The test needs 2 successful connections to
other peers (two Python `P2PInterface`s) and they work in spite of the
unavailable proxy because they are on `127.0.0.1` (`NET_UNROUTABLE`) and
the proxy is not used for that.
`feature_config_args.py` uses a proxy address of `1.2.3.4`. This results
in actually trying to open TCP connections over the internet to
`1.2.3.4:9050`.
The test does not need those to succeed so use `127.0.0.1:1` instead.
Also avoid `-noconnect=0` because that is interpreted as `-connect=1`
which is interpreted as `-connect=0.0.0.1` and a connection to
`0.0.0.1:18444` is attempted.
`p2p_seednode.py` would try to connect to `0.0.0.1` and `0.0.0.2` as
seed nodes. This sends outbound TCP packets on a non-loopback interface
to the default router.
Configure an unavailable proxy for all executions of `bitcoind` during
this test. Also change `0.0.0.1` and `0.0.0.2` because connecting to
them would skip the `-proxy=` setting because for such an address:
* `CNetAddr::IsLocal()` is true, thus
* `CNetAddr::IsRoutable()` is false, thus
* `CNetAddr::GetNetwork()` is `NET_UNROUTABLE`, even though
`CNetAddr::m_net` is `NET_IPV4`.
This speeds up the execution time of `p2p_seednode.py`
from 12.5s to 2.5s.
69e95c2b4f tests: Test cleanup of mkeys from wallets without privkeys (Andrew Chow)
2b9279b50a wallet: Remove unused encryption keys from watchonly wallets (Andrew Chow)
813a16a463 wallet: Add HasCryptedKeys (Andrew Chow)
Pull request description:
An earlier version allowed users to create watchonly wallets (wallets without private keys) that were "encrypted". Such wallets would have a stored encryption keys, but nothing would actually be encrypted with them. This can cause unexpected behavior such as https://github.com/bitcoin-core/gui/issues/772.
We can detect such wallets as they will have the disable private keys flag set, no encrypted keys, and encryption keys. For such wallets, we can remove those encryption keys thereby avoiding any issues that may result from this unexpected situation.
ACKs for top commit:
sipa:
utACK 69e95c2b4f.
laanwj:
Code review re-ACK 69e95c2b4f
furszy:
Code review ACK 69e95c2b4f
Tree-SHA512: 901932cd709c57e66c598f011f0105a243b5a8b539db2ef3fcf370dca4cf35ae09bc1110e8fca8353be470f159468855a4dd96b99bc9c1112adc86ccc50e1b9d
589ed1a8ea wallet: migration, avoid loading wallet after failure when it wasn't loaded before (furszy)
Pull request description:
Fixes#31447.
During migration failure, only load wallet back into memory when the wallet was
loaded prior to migration. This fixes the case where BDB is not supported, which
implies that no legacy wallet can be loaded into memory due to the lack of db
writing functionality.
Link to error description https://github.com/bitcoin/bitcoin/issues/31447#issuecomment-2528757140.
This PR also improves migration backup related comments to better document the
current workflow.
ACKs for top commit:
achow101:
ACK 589ed1a8ea
rkrux:
ACK 589ed1a8ea
pablomartin4btc:
tACK 589ed1a8ea
Tree-SHA512: c7a489d2b253c574ee0287b691ebe29fe8d026f659f68a3f6108eca8b4e1e420c67ca7803c6bd70c1e1440791833fabca3afbcf8fe8524c6c9fc08de95b618d0
If the `releases` directory exists, but still only a subset of the
necessary previous release binaries are available, the test fails by
throwing an exception (sometimes leading to follow-up exceptions like
"AssertionError: [node 0] Error: no RPC connection") and printing out
a stack trace, which can be confusing and at a first glance suggests
that the node crashed or some alike.
Improve this by checking and printing out *all* of the missing release
binaries and failing with an explicit error in this case. Also add an
info on how to download previous releases binaries.
Noticed while testing #30328.
Can be tested by e.g.
$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py
Now that we track all announcers of an orphan, it's not helpful to
consider an orphan provided by a peer that didn't send us this parent.
It can only hurt our chances of finding the right orphan when there are
multiple candidates.
Adapt the 2 tests in p2p_opportunistic_1p1c.py that looked at 1p1c
packages from different peers. Instead of checking that the right peer
is punished, we now check that the package is not submitted. We can't
use the functional test to see that the package was not considered
because the behavior is indistinguishable (except for the logs).
e8f0e6efaf lint: output-only - Avoid repeated arrows, trim (Hodlinator)
fa9aacf614 lint: Move assertion linter into lint runner (MarcoFalke)
Pull request description:
On failure, this makes the output more consistent with the other linters. Each failure will be marked with an '⚠️ ' emoji and explanation, making it easier to spot.
Also, add --line-number to the filesystem linter.
Also, add newlines after each failing check, to visually separate different failures from each other.
Can be reviewed with:
`--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
ACKs for top commit:
davidgumberg:
crACK e8f0e6efaf
hodlinator:
re-ACK e8f0e6efaf
TheCharlatan:
ACK e8f0e6efaf
Tree-SHA512: 9896ff882af9d673ec3e6d2718f877b2fdc8514faba50942fcebacb9de95b1f5b4a5db595e1338fa7f505d06df2df304897350cc55c558c7a85232800e5fd804
b6f0593f43 doc: add release note about testmempoolaccept debug-message (Matthew Zipkin)
f9cac63523 test: cover testmempoolaccept debug-message in RBF test (Matthew Zipkin)
f9650e18ea rbf: remove unecessary newline at end of error string (Matthew Zipkin)
221c789e91 rpc: include verbose reject-details field in testmempoolaccept response (Matthew Zipkin)
Pull request description:
Adds a new field `reject-details` in `testmempoolaccept` responses to include `m_debug_message` from `ValidationState`. This string is the complete error message thrown by the mempool in response to `sendrawtransaction`.
The extra verbosity is helpful to consumers of `testmempoolaccept`, which is sort of a debug tool anyway.
example:
>
> {
> "txid": "07d7a59a7bdad4c3a5070659ea04147c9b755ad9e173c52b6a38e017abf0f5b8",
> "wtxid": "5dc243b1b92ee2f5a43134eb3e23449be03d1abb3d7f3c03c836ed0f13c50185",
> "allowed": false,
> "reject-reason": "insufficient fee",
> "reject-details": "insufficient fee, rejecting replacement 07d7a59a7bdad4c3a5070659ea04147c9b755ad9e173c52b6a38e017abf0f5b8; new feerate 0.00300000 BTC/kvB <= old feerate 0.00300000 BTC/kvB"
> }
ACKs for top commit:
rkrux:
re-ACK b6f0593f43
glozow:
ACK b6f0593f43
Tree-SHA512: 340b8023d59cefa84598879c4efdb7c399a3f62da126e87c595523f302e53d33098fc69da9c5f8c92b7580dc75466c66cea372051f935b197265648fe15c43a3
fa63b8232f test: generateblocks called by multiple threads (MarcoFalke)
fa62c8b1f0 rpc: Extend scope of validation mutex in generateblock (MarcoFalke)
Pull request description:
The mutex (required by TestBlockValidity) must be held after creating the block, until TestBlockValidity is called. Otherwise, it is possible that the chain advances in the meantime and leads to a crash in TestBlockValidity: `Assertion failed: pindexPrev && pindexPrev == chainstate.m_chain.Tip() (validation.cpp: TestBlockValidity: 4338)`
Fixes#31562
ACKs for top commit:
davidgumberg:
reACK fa63b8232f
achow101:
ACK fa63b8232f
ismaelsadeeq:
re-ACK fa63b8232f
mzumsande:
utACK fa63b8232f
Tree-SHA512: 3dfda1192af52546ab11fbffe44af8713073763863f4a63fbcdbdf95b1c6cbeb003dc4b8b29e7ec67362238ad15e07d8f6855832a0c68dc5370254f8cbf9445c
bc43ecaf6d test: add functional test for balance after snapshot completion (Martin Zumsande)
226d03dd61 validation: Send correct notification during snapshot completion (Martin Zumsande)
Pull request description:
After AssumeUtxo background sync is completed in a `ActivateBestChain()` call, the `GetRole()` function called with `BlockConnected()` returns `ChainstateRole::NORMAL` instead of `ChainstateRole::BACKGROUND` for this chainstate.
This would make the wallet (which ignores `BlockConnected` notifications for the background chainstate) process it, change `m_last_block_processed_height` to the (ancient) snapshot height, and display an incorrect balance.
Fix this by caching the chainstate role before calling `ActivateBestChainStep()`.
Also contains a test for this situation that fails on master.
Fixes#31546
ACKs for top commit:
fjahr:
re-ACK bc43ecaf6d
achow101:
ACK bc43ecaf6d
furszy:
Code review ACK bc43ecaf6d
TheCharlatan:
lgtm ACK bc43ecaf6d
Tree-SHA512: c5db677cf3fbab3a33ec127ec6c27c8812299e8368fd3c986bc34d0e515c4eb256f6104479f27829eefc098197de3af75d64ddca636b6b612900a0e21243e4f2
fa0998f0a0 test: Avoid intermittent error in assert_equal(pruneheight_new, 248) (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/31446
The test uses the P2P network to sync blocks, which has no inherent guarantee that the blocks are sent and received in the right order, assuming the headers are received first.
This can mean that the first block file is flushed with block at height 249 and block at height 248 is added to the second file. In the log it looks like: `Leaving block file 0: CBlockFileInfo(blocks=249, size=65319, heights=0...249, time=2011-02-02...2024-12-03) (onto 1) (height 248)`. The test assumes that the height of the last pruned block in the first file is 248, expecting it to look like: `Leaving block file 0: CBlockFileInfo(blocks=249, size=65319, heights=0...248, time=2011-02-02...2024-12-09) (onto 1) (height 249) `.
Fix the issue by using a linear dumb sync.
ACKs for top commit:
achow101:
ACK fa0998f0a0
mzumsande:
Code Review ACK fa0998f0a0
i-am-yuvi:
Code Review ACK fa0998f0a0
fjahr:
Code review ACK fa0998f0a0
Tree-SHA512: 59cb4317be6cf9012c9bf7a3e9f5ba96b8b114b30bd2ac42af4fe742cd26a634d685b075f04a84bd782b2a43a342d75bb20a042bd82ad2831dbf844d39517ca2
fa6e599cf9 test: Call generate through test framework only (MarcoFalke)
Pull request description:
The generate RPCs are special in that they should only be called by the test framework itself. This way, they will call the sync function on the nodes, which can avoid intermittent test issues. Also, when the sync is disabled, it will happen explicitly by setting the `sync_fun`.
Apply this rule here, so that all generate calls are written consistently.
ACKs for top commit:
achow101:
ACK fa6e599cf9
rkrux:
tACK fa6e599cf9
hodlinator:
ACK fa6e599cf9
i-am-yuvi:
Tested ACK fa6e599cf9
Tree-SHA512: 31079997f1e17031ecd577904457e0560388aa53cadb1bbda281865271e8e4cf244bc6bf315838a717bf9d6620c201093e30039aa0007bec3629f7ca56abfba3
a2c45ae548 test: report failure during utf8 response decoding (furszy)
Pull request description:
Useful for debugging issues such https://github.com/bitcoin/bitcoin/pull/31241#issuecomment-2462816933.
Prints the entire response content instead of printing only the position of the byte it can't be decoded.
The diff between the error messages can be seen by running the `wallet_migration.py` functional test with the following patch applied:
```
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
--- a/src/wallet/rpc/wallet.cpp(revision d65918c5da52c7d5035b4151dee9ffb2e94d4761)
+++ b/src/wallet/rpc/wallet.cpp(date 1731005254673)
@@ -801,7 +801,7 @@
}
UniValue r{UniValue::VOBJ};
- r.pushKV("wallet_name", res->wallet_name);
+ r.pushKV("wallet_name", "\xc3\x28");
if (res->watchonly_wallet) {
r.pushKV("watchonly_name", res->watchonly_wallet->GetName());
}
```
ACKs for top commit:
achow101:
ACK a2c45ae548
theStack:
re-ACK a2c45ae548
rkrux:
tACK a2c45ae548
ismaelsadeeq:
utACK a2c45ae548
Tree-SHA512: 6abb524b5a215c51ec881eea91ebe8174140a88ff3874c8c88676157edae7818801356586a904dbb21b45053183315a6d71dbf917d753611d8e413776b57c484
ecaa786cc1 rpc: add signet_challenge field to getblockchaininfo and getmininginfo (Ash Manning)
Pull request description:
Signet challenges are currently only available via `getblocktemplate` RPC.
`getblockchaininfo` and `getmininginfo` both provide inadequate information to distinguish signets. Since these are the RPCs used to determine the current network, they should also provide the signet challenge for signets.
Test coverage is included in `test/functional/feature_signet.py`.
ACKs for top commit:
sipa:
utACK ecaa786cc1
achow101:
ACK ecaa786cc1
i-am-yuvi:
Concept ACK ecaa786cc1
Sjors:
ACK ecaa786cc1
zaidmstrr:
Tested ACK [ecaa786](ecaa786cc1)
Tree-SHA512: 9ccf4ae634ee74353a2a895efb881fdc62ae703a134ccd219da2cd6080c7d38319e689054584722457a7cc79004bd6022292a3b0b90eaab9f7003564665e1ea4
06443b8f28 net: clarify if we ever sent or received from peer (Sjors Provoost)
1d01ad4d73 net: add LogIP() helper, use in net_processing (Sjors Provoost)
937ef9eb40 net_processing: use CNode::DisconnectMsg helper (Sjors Provoost)
ad224429f8 net: additional disconnection logging (Sjors Provoost)
Pull request description:
While debugging unexpected disconnections, possibly related to #28331, I found some additional [net] logging to be useful.
All cases where we disconnect now come with a log message that has the word `disconnecting`:
* all calls to `CloseSocketDisconnect()` log `disconnecting peer=…`
* wherever we set `pnode->fDisconnect = true;`
* for all `InactivityCheck` cases (which in turn sets `fDisconnect`)
* replaces "dropping" with "disconnecting" in `Network not active, dropping peer=…`
A few exceptions are listed here: https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890824361
I changed `CloseSocketDisconnect()` to no longer log `disconnecting`, and instead have all the call sites do so.
This PR introduces two helper functions on `CNode`: `DisconnectMsg` and `LogIP`. The second and third commit use these helpers in `net_processing.cpp` so these disconnect messages are more consistent now (e.g. some didn't log the IP). No new messages are added there though.
The `LogIP()` helper is rarely used outside of a disconnect event, but it's available for future use.
Any `LogPrint` this PR touches is replaced with `LogDebug` (superseded by #30750), and every `LogPrintf ` with `LogInfo`.
ACKs for top commit:
davidgumberg:
reACK 06443b8f28
vasild:
ACK 06443b8f28
danielabrozzoni:
ACK 06443b8f28
hodlinator:
ACK 06443b8f28
Tree-SHA512: 525f4c11568616e1d48455a3fcab9e923da7432377fe9230468c15403d2e9b7ce712112df8fbd547cfec01dce0d1f26107cfc1b90f78cfc1fe13e08d57b08464
fa0e30b93a fuzz: Fix test_runner error reporting (MarcoFalke)
Pull request description:
The error reporting is confusing, because right now it prints:
https://cirrus-ci.com/task/4846031060336640?logs=ci#L4931
```
...
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 411, in <module>
main()
File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 199, in main
run_once(
File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/fuzz/test_runner.py", line 376, in run_once
assert len(done_stat) == 1
^^^^^^^^^^^^^^^^^^^
AssertionError
```
This is harmless, but confusing.
Fix it by collecting statistics only when the program has not aborted. (Can be reviewed with `--color-moved=dimmed-zebra`)
Also, reword the error message to align it with error messages in other test_runners in this repo.
ACKs for top commit:
dergoegge:
utACK fa0e30b93a
brunoerg:
code review ACK fa0e30b93a
marcofleon:
Tested ACK fa0e30b93a. Prints out the error for the target that crashed. Much clearer than the current error message.
Tree-SHA512: 5e8d3fc0e4837b3264ff0c3cb322fe7fe2ec7af48d35e2a14f82080d03ace793963c3314611b0a170a38e200497d7ba703d9c35c9a7ed3272d93e43f0f0e4c2b
1dd3af8fbc Add release note for #31223 (Martin Zumsande)
997757dd2b test: add functional test for -port behavior (Martin Zumsande)
0e2b12b92a net, init: derive default onion port if a user specified a -port (Martin Zumsande)
Pull request description:
This resolves#31133 (setups with multiple local nodes each using a different `-port` no longer working with v28.0, see the issue description for more details) by deriving the default onion listening port to be the value specified by `-port` incremented by 1 (idea by vasild / laanwj).
Note that with this fix, the chosen `-port` values of two local nodes cannot be adjacent, otherwise there will be port collisions again.
From the discussion in the linked issue, this was the most popular option, followed by doing nothing and telling affected users to change their setups to use `-bind` instead of `-port`. But more opinions are certainly welcome!
I think that if we decide to do something about the problem described in the issue, we should do so soon (in 28.1.), so I opened this PR.
Fixes#31133
ACKs for top commit:
achow101:
ACK 1dd3af8fbc
laanwj:
Tested ACK 1dd3af8fbc
tdb3:
Code review ACK 1dd3af8fbc
Tree-SHA512: 37fda2b23bbedcab5df3a401cf5afce66ae5318fb78f9660f83e3fd075b528e8156d7a0903f9a12ffe97ab5d83860587116b74af28670a1f4c2f0d1be4999f40
- No empty line separating errors and arrows ("^^^"). Keeping them together signals they are related.
- No empty line separating error message and linter failure line (not completely empty, it contains several spaces left over from Rust multi-line literal).
- Keep the linter description on the same line as the failure line, otherwise it looks like it's a description for the following step.
62b2d23edb wallet: Migrate non-HD keys to combo() descriptor (Ava Chow)
Pull request description:
Non-HD keys do not have an HD seed ID associated with them, so if this value is the null value (all 0s), then we should not perform any seed ID comparison that would result in excluding the keys from combo() migration.
This changes the migration of non-HD wallets (or blank wallets with imported private keys) to make a single combo() descriptors for the non-HD/imported keys, rather than pk(), pkh(), sh(wpkh()), and wpkh() descriptors for the keys.
Implements https://github.com/bitcoin/bitcoin/pull/31374#discussion_r1876650074
ACKs for top commit:
laanwj:
Concept and code review ACK 62b2d23edb
brunoerg:
code review ACK 62b2d23edb
furszy:
Nice catch. ACK 62b2d23edb
theStack:
ACK 62b2d23edb
rkrux:
tACK 62b2d23edb
Tree-SHA512: 86a80b7dcc1598ab18068a2572ff4b4920b233178b760f7b76c5b21a9e6608005ac872f90e082a8f99b51daab0b049e73e4bee5b8e0b537d56ed0d34122a1f49
On failure, this makes the output more consistent with the other linter.
Each failure will be marked with an '⚠️ ' emoji and explanation, making
it easier to spot.
Also, add --line-number to the filesystem linter.
Also, add newlines after each failing check, to visually separate
different failures from each other.
Can be reviewed with:
"--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space"
During migration failure, only load wallet back into memory when the
wallet was loaded prior to migration. This fixes the case where BDB
is not supported, which implies that no legacy wallet can be loaded
into memory due to the lack of db writing functionality.
This commit also improves migration backup related comments to better
document the current workflow.
Co-authored-by: Ava Chow <github@achow101.com>
c93bf0e6e2 test: Add missing %c character test (Hodlinator)
76cca4aa6f test: Document non-parity between tinyformat and ConstevalFormatstring (Hodlinator)
533013cba2 test: Prove+document ConstevalFormatString/tinyformat parity (Hodlinator)
b81a465995 refactor test: Profit from using namespace + using detail function (Hodlinator)
Pull request description:
Clarifies and puts the extent of parity under test.
Broken out from #30546 based on https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263 and https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304.
ACKs for top commit:
maflcko:
re-ACK c93bf0e6e2 🗜
l0rinc:
ACK c93bf0e6e2
ryanofsky:
Code review ACK c93bf0e6e2. Just a few cleanups tweaking function declarations and commit comments and consolidating some test cases since last review.
Tree-SHA512: 5ecc893b26cf2761c0009861be392ec4c4fceb0ef95052a2f6f9df76b2e459cfb3f9e257f61be07c3bb2ecc6e525e72c5ca853be1f63b70b52785323d3db6b42
988721d37a test: avoid internet traffic in rpc_net.py (Sebastian Falbesoner)
Pull request description:
In order to avoid connecting to the internet in the functional test `rpc_net.py`, specify a non-working proxy (parameter `-proxy=127.0.0.1:1`, same approach as in #31142) for the nodes. There is at least one known instance where this is currently happening on master where a connection attempt to a public IP is made (see also the discussion in #31339):
17834bd197/test/functional/rpc_net.py (L253)
Can be tested by running
```
$ sudo tcpdump -i eth0 host 11.22.33.44
```
both on master and the PR branch and verifying that no packets appear in the tcpdump in the latter anymore.
ACKs for top commit:
achow101:
ACK 988721d37a
tdb3:
ACK 988721d37a
vasild:
ACK 988721d37a
Tree-SHA512: 0f51fedccbfac0f80a7e6f9c5ba9193d0c20b5a788553c7cd7e583225df7b1151b86cd848d6ccf61f7b2de848f0ac98d73d7b5db100aa54fe8cbeeb4c0549106
ec777917d6 test: Fix intermittent issue in wallet_backwards_compatibility.py (Randall Naar)
Pull request description:
When creating and replacing a transaction using `bumpfee`, an async update is sent in the form of the `TransactionAddedToMempool` and `TransactionRemovedFromMempool` signals. When `wallet_backwards_compatibility.py` creates `tx3_id` this way and replaces it with `tx4_id`, the `abandontransaction` rpc is called right after. In some cases the `TransactionAddedToMempool` and `TransactionRemovedFromMempool` is handled after the transaction is abandoned in the wallet, and overwrites the transaction's `abandoned` flag. This PR forces the signals to get handled before `abandontransaction` is called by invoking `self.sync_mempools` which calls `syncwithvalidationinterfacequeue` on every node's rpc connection.
This will mitigate the immediate inconsistency observed with the abandontransaction call, but the potential race conditions between the signals and wallet operations may also be useful to note in a separate issue (if it's okay to not address it in this one).
Fixes#29806
ACKs for top commit:
achow101:
ACK ec777917d6
tdb3:
ACK ec777917d6
Tree-SHA512: e75bc2c1f7fefc4f4910bb353654848fed5661c1436416798a5f4e0c5a76bde15617a5af04c2384464005953326317b8f273039e47508d5124677908cf36d31e
Non-HD keys in legacy wallets without a HD seed ID were being migrated
to separate pk(), pkh(), sh(wpkh()), and wpkh() descriptors for each key.
These could be more compactly represented as combo() descriptors, so
migration should make combo() for them.
It is possible that existing non-HD wallets that were migrated, or
wallets that started blank and had private keys imported into them have
run into this issue. However, as the 4 descriptors produce the same output
scripts as the single combo(), so any previously migrated wallets are
not missing any output scripts. The only observable difference should be
performance related, and the wallet size on disk.
cdd207c0e4 test: add coverage for migrating standalone imported keys (furszy)
297a876c98 test: add coverage for migrating watch-only script (furszy)
932cd1e92b wallet: fix crash during watch-only wallet migration (furszy)
Pull request description:
The crash occurs because we assume the cached scripts structure will not be empty,
but it can be empty for watch-only wallets that start blank.
This also adds test coverage for standalone imported keys, which were also crashing
because pubkey imports are treated the same way as hex script imports through
`importaddress()`.
Testing Notes:
This can be verified by cherry-picking and running any of the test commits on master.
It will crash there but pass on this branch.
ACKs for top commit:
theStack:
re-ACK cdd207c0e4
brunoerg:
reACK cdd207c0e4
achow101:
ACK cdd207c0e4
Tree-SHA512: e05c77cf3e9f35f10f122a73680b3f131f683c56685c1e26b5ffc857f95195b64c8c9d4535960ed3d6f931935aa79b0b1242537462006126bdb68251f0452954
- For "%n", which is supposed to write to the argument for printf.
- For string/integer mismatches of width/precision specifiers.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
