✅ achow101 closed an issue: "Test case for spending bare multisig?"
(https://github.com/bitcoin/bitcoin/issues/29113)

🚀 achow101 merged a pull request: "test: Add test case for spending bare multisig"
(https://github.com/bitcoin/bitcoin/pull/29120)

💬 glozow commented on pull request "test: Don't rely on incentive incompatible replacement in mempool_accept_v3.py":
(https://github.com/bitcoin/bitcoin/pull/29986#discussion_r1586637512)
above:
- `fee_to_beat_absolute` is 41600 (above code uses `max(tx_unrelated_replacee, tx_v3_child_2)` but Rule 3 would require us to beat the sum of their fees haha)
- `feerate_to_beat` is 300, i.e. 31200 /104
- `fee_to_beat_feerate` is 300 * (147 + 154) = 90300
- `fee_v3_child_3` is max(41600, 90300) + 5 = 90305
- this means the ancestor feerate is 90305 / (147 + 154) = 300.017

💬 instagibbs commented on pull request "fuzz: txorphan tests fixups":
(https://github.com/bitcoin/bitcoin/pull/29974#discussion_r1586607474)
micro-nit/guess: for stability I think a single boolean might be better

👍 instagibbs approved a pull request: "fuzz: txorphan tests fixups"
(https://github.com/bitcoin/bitcoin/pull/29974#pullrequestreview-2034172340)
ACK 58594c7040241f2312b0b8735a8baf0412674613

ran fuzz for a while, no issues

🤔 stickies-v reviewed a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2033751635)
Concept ACK, but I'm concerned about the behaviour change in `AlreadyHaveTx`, which seems quite non-trivial to review so I'll need to look into further.

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1586638555)
Or alternatively:

<details>
<summary>git diff on 4b4dfaa8f3</summary>

```diff
diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp
index 504a1f7ec3..bed92e6b32 100644
--- a/src/txorphanage.cpp
+++ b/src/txorphanage.cpp
@@ -98,13 +98,11 @@ void TxOrphanage::EraseForPeer(NodeId peer)
m_peer_work_set.erase(peer);

int nErased = 0;
- std::map<Wtxid, OrphanTx>::iterator iter = m_orphans.begin();
- while (iter != m_orphans.end())
- {
- std::map<Wtxid, Or
...

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1586533521)
nit: can just one-line
```suggestion
Assert(orphanage.HaveTx(ref->GetWitnessHash()));
```

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1586713579)
It's not immediately obvious to me why it is safe to just convert any `gtxid` into a `Wtxid` here, especially since there are callsites of `AlreadyHaveTx` where a ` GenTxId::Txid` is passed, such as e.g. [here](https://github.com/bitcoin/bitcoin/blob/d73245abc70346a0e8805d50a1f395706084294c/src/net_processing.cpp#L4632). If it is indeed safe, I think it could be a better approach to update `AlreadyHaveTx` to take a `Wtxid` and push the conversion further to the edge, so it can be reviewed on a c
...

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1586372606)
```suggestion
/** Check if we already have an orphan transaction */
```

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1586410875)
nit: looks like this is only used for logging (and thus can be declared right before the logging) - do we still want/need to log the txid? I'd think wtxid suffices? But nvm if that's contentious.

💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1586417487)
nit: can't we just use `first`?
```suggestion
nErased += EraseTxNoLock(maybeErase->first);
```

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2088971908)
re: https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-2080843513

Thanks @stickies-v. I think since `Update()` is not required immediately I might move the first and third commits of this PR to another PR without `Update()`. The only downside will be some churn in the `CompleteChainstateInitialization` function, since I believe at some point it will need to be changed again to use the `Update()` pattern or a similar pattern, when other functions it calls are changed to return `util::R
...

👍 instagibbs approved a pull request: "opportunistic 1p1c followups"
(https://github.com/bitcoin/bitcoin/pull/30012#pullrequestreview-2034310688)
ACK 6119f76ef72c1adc55c1a384c20f8ba9e1a01206

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-2088994389)
@ryanofsky thanks so much, again, for your help. Especially in understanding the `std::move` semantics. I've reviewed each commit in your branch and force pushed it to mine with the only change being specifying json request version in `JSONRPCReplyObj()` calls in `bitcoin-cli.cpp`

👍 TheCharlatan approved a pull request: "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29494#pullrequestreview-2034337579)
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3

💬 TheCharlatan commented on pull request "init: Fixes for file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/27539#issuecomment-2089013169)
This was rebased fairly recently, but you have not addressed this open comment https://github.com/bitcoin/bitcoin/pull/27539#discussion_r1203666492 . Are you still working on this?

💬 achow101 commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2089055814)
ACK 84900ac34f6888b7a851d0a6a5885192155f865c

Tested on Windows. `%APPDATA%/Bitcoin` continues to be used if it exists, `%LOCALAPPDATA%` if not.

💬 achow101 commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#issuecomment-2089058662)
re-ACK 42fb5311b19582361409d65c6fddeadbee14bb97

🤔 sr-gi reviewed a pull request: "net: don't lock cs_main while reading blocks in net processing"
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-2034387611)
Concept ACK

I agree with @furszy in https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1381776328 that we should followup this with sending NotFounds for all the blocks that we are unable (or unwilling) to provide