💬 stickies-v commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2239805692)
nit: `txid` declaration not really helpful imo
```suggestion
const Wtxid& wtxid{txinfo.tx->GetWitnessHash()};
const auto inv = peer->m_wtxid_relay ?
CInv{MSG_WTX, wtxid.ToUint256()} :
CInv{MSG_TX, txinfo.tx->GetHash().ToUint256()};
```
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2239805692)
nit: `txid` declaration not really helpful imo
```suggestion
const Wtxid& wtxid{txinfo.tx->GetWitnessHash()};
const auto inv = peer->m_wtxid_relay ?
CInv{MSG_WTX, wtxid.ToUint256()} :
CInv{MSG_TX, txinfo.tx->GetHash().ToUint256()};
```
💬 stickies-v commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2240488569)
tl;dr - I think the current implementation 94b39ce73831acc4c94c7f0d1347d5991b27ef0b (i.e. ajtown's suggestion in https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2224354639) is pragmatically the best way forward.
---
> `GetIter` is already exposed
I think "still" is more appropriate than "already", see e.g. work done in #28391. Non-mempool code using `mapTx` iterators imo remains an anti-pattern and we should strive to reduce, not increase it.
> That removes p2p-specific cod
...
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2240488569)
tl;dr - I think the current implementation 94b39ce73831acc4c94c7f0d1347d5991b27ef0b (i.e. ajtown's suggestion in https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2224354639) is pragmatically the best way forward.
---
> `GetIter` is already exposed
I think "still" is more appropriate than "already", see e.g. work done in #28391. Non-mempool code using `mapTx` iterators imo remains an anti-pattern and we should strive to reduce, not increase it.
> That removes p2p-specific cod
...
💬 stickies-v commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2240139278)
ghost nit
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2240139278)
ghost nit
💬 stickies-v commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2240034170)
nit: this is touching a lot more code than you are, but modernizing this bit of code would make it more readable:
<details>
<summary>git diff on 94b39ce738</summary>
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 165f6e822f..a9dafb7160 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5780,28 +5780,23 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
// Determine transactions to relay
if (fSendTrick
...
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2240034170)
nit: this is touching a lot more code than you are, but modernizing this bit of code would make it more readable:
<details>
<summary>git diff on 94b39ce738</summary>
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 165f6e822f..a9dafb7160 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -5780,28 +5780,23 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
// Determine transactions to relay
if (fSendTrick
...
💬 l0rinc commented on pull request "assumevalid: log every script validation state change":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2240517592)
Yeah, I figured, but this way the scope of `p2p1` is narrowed to a single block - seems more consistent than breaking it up
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2240517592)
Yeah, I figured, but this way the scope of `p2p1` is narrowed to a single block - seems more consistent than breaking it up
💬 l0rinc commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3133445490)
I don't see how a comment change could cause the new CI failure
reACK caea5f0bbf67984b3a187334ceef9e3023175848
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3133445490)
I don't see how a comment change could cause the new CI failure
reACK caea5f0bbf67984b3a187334ceef9e3023175848
🤔 w0xlt reviewed a pull request: "doc: Add legacy wallet removal release notes"
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3068599342)
ACK https://github.com/bitcoin/bitcoin/pull/33075/commits
(https://github.com/bitcoin/bitcoin/pull/33075#pullrequestreview-3068599342)
ACK https://github.com/bitcoin/bitcoin/pull/33075/commits
💬 achow101 commented on pull request "doc: Add legacy wallet removal release notes":
(https://github.com/bitcoin/bitcoin/pull/33075#issuecomment-3133503651)
ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
(https://github.com/bitcoin/bitcoin/pull/33075#issuecomment-3133503651)
ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
🚀 achow101 merged a pull request: "doc: Add legacy wallet removal release notes"
(https://github.com/bitcoin/bitcoin/pull/33075)
(https://github.com/bitcoin/bitcoin/pull/33075)
💬 achow101 commented on pull request "wallet: Fix relative path backup during migration.":
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3133523088)
I'm going to go ahead and merge this now as @davidgumberg is out for the next couple weeks. Remaining comments can be addressed in a followup.
(https://github.com/bitcoin/bitcoin/pull/32273#issuecomment-3133523088)
I'm going to go ahead and merge this now as @davidgumberg is out for the next couple weeks. Remaining comments can be addressed in a followup.
💬 achow101 commented on pull request "doc: add note for watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32866#issuecomment-3133529680)
ACK 5888b4a2a5566c64141b78a0e7660a166ec99775
(https://github.com/bitcoin/bitcoin/pull/32866#issuecomment-3133529680)
ACK 5888b4a2a5566c64141b78a0e7660a166ec99775
💬 maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3133536724)
The checkout action by default picks the "rebase"/merge with master, so this should happen even without you rebasing. In fact, it should happen even when not pushing at all, but just re-running the task. However, I just noticed this does not happen (https://github.com/bitcoin/bitcoin/actions/runs/16521447783/job/46969566419?pr=29641#step:2:90 picks a 4-day old merge). This breaks the use-case of being able to detect silent merge conflicts before a maintainer detects them (or misses them).
Not
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3133536724)
The checkout action by default picks the "rebase"/merge with master, so this should happen even without you rebasing. In fact, it should happen even when not pushing at all, but just re-running the task. However, I just noticed this does not happen (https://github.com/bitcoin/bitcoin/actions/runs/16521447783/job/46969566419?pr=29641#step:2:90 picks a 4-day old merge). This breaks the use-case of being able to detect silent merge conflicts before a maintainer detects them (or misses them).
Not
...
🚀 achow101 merged a pull request: "wallet: Fix relative path backup during migration."
(https://github.com/bitcoin/bitcoin/pull/32273)
(https://github.com/bitcoin/bitcoin/pull/32273)
💬 achow101 commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3133584656)
> How is it different than any other multi-process binary?
The multiprocess binaries are produced by this project, built using the same deterministic build system, using the same code as the monolithic binaries. There are already concrete ways that the binaries can be distributed with minimal changes to our own release process. Users who run the multiprocess binaries don't have to do any special configuration for them to work. This is different from requiring users to set a configuration opti
...
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3133584656)
> How is it different than any other multi-process binary?
The multiprocess binaries are produced by this project, built using the same deterministic build system, using the same code as the monolithic binaries. There are already concrete ways that the binaries can be distributed with minimal changes to our own release process. Users who run the multiprocess binaries don't have to do any special configuration for them to work. This is different from requiring users to set a configuration opti
...
⚠️ jonatack opened an issue: "spurious failure in p2p_leak.py --v1transport"
(https://github.com/bitcoin/bitcoin/issues/33090)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
https://github.com/bitcoin/bitcoin/actions/runs/16601603414/job/46962514184?pr=31886
### Expected behaviour
This test should have passed.
### Steps to reproduce
See logs in
https://github.com/bitcoin/bitcoin/actions/runs/16601603414/job/46962514184?pr=31886
I cannot reproduce it locally.
### Relevant log output
_No response_
### How did you obtain Bitcoin Core
Compiled from sourc
...
(https://github.com/bitcoin/bitcoin/issues/33090)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
https://github.com/bitcoin/bitcoin/actions/runs/16601603414/job/46962514184?pr=31886
### Expected behaviour
This test should have passed.
### Steps to reproduce
See logs in
https://github.com/bitcoin/bitcoin/actions/runs/16601603414/job/46962514184?pr=31886
I cannot reproduce it locally.
### Relevant log output
_No response_
### How did you obtain Bitcoin Core
Compiled from sourc
...
💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3133622663)
Yes, I opened an issue to report it: https://github.com/bitcoin/bitcoin/issues/33090
I don't see how to re-start that test.
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3133622663)
Yes, I opened an issue to report it: https://github.com/bitcoin/bitcoin/issues/33090
I don't see how to re-start that test.
💬 luke-jr commented on pull request "init: [gui] Avoid UB/crash in InitAndLoadChainstate":
(https://github.com/bitcoin/bitcoin/pull/32987#issuecomment-3133628881)
Alternative to the test option, Core could just add support for reindex=auto (#22072)
(https://github.com/bitcoin/bitcoin/pull/32987#issuecomment-3133628881)
Alternative to the test option, Core could just add support for reindex=auto (#22072)
🚀 achow101 merged a pull request: "doc: add note for watch-only wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32866)
(https://github.com/bitcoin/bitcoin/pull/32866)
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2240683030)
The cache can only be generated if private keys are available, so we need this to write the cache to disk of the watchonly wallet.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2240683030)
The cache can only be generated if private keys are available, so we need this to write the cache to disk of the watchonly wallet.
💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2240684156)
I think export is a reasonable name since listing them is also an export.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2240684156)
I think export is a reasonable name since listing them is also an export.