💬 delta1 commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3133195798)
@darosior thanks I intend to make fee estimation work correctly as well

💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2240344928)
good eye, done

💬 theuni commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3133226979)
Looks like https://github.com/llvm/llvm-project/issues/111048 to me?

💬 theuni commented on pull request "rpc: use CScheduler for relocking wallet and remove RPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32862#issuecomment-3133245232)
Nm, I misunderstood that report. We don't have a shared lib missing instrumentation.

💬 ismaelsadeeq commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3133306000)
re-ACK e1f139bd5fc9ee737d2b08307fca2b33354c5747

> I feel like adding more special cases to this code will make it less less useful as a demonstration of what the mining API looks like and how it can be called.

Previously, I assumed that the purpose of this commit was to demonstrate how the mining API works. @Sjors highlighted it's for test coverage here https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3087403014. Your recent comment seems to reinforce my original assumption.


...

💬 Sjors commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3133365936)
re-ACK e1f139bd5fc9ee737d2b08307fca2b33354c5747

👍 stickies-v approved a pull request: "refactor: GenTxid type safety followups"
(https://github.com/bitcoin/bitcoin/pull/33005#pullrequestreview-3067511438)
ACK 94b39ce73831acc4c94c7f0d1347d5991b27ef0b

💬 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()};
```

💬 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
...

💬 stickies-v commented on pull request "refactor: GenTxid type safety followups":
(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
...

💬 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

💬 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

🤔 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

💬 achow101 commented on pull request "doc: Add legacy wallet removal release notes":
(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)

💬 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.

💬 achow101 commented on pull request "doc: add note for watch-only wallet migration":
(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
...

🚀 achow101 merged a pull request: "wallet: Fix relative path backup during migration."
(https://github.com/bitcoin/bitcoin/pull/32273)