👍 theStack approved a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2055922168)
ACK 0fb17bf61a40b73a2b81a18e70b3de180c917f22

I like the latest, more verbose version of the first commit (7e475b9648bbee04f5825b922ba0399373eaa5a9), as it was significantly easier to grasp what's going on and convince myself that this is the behaviour we want. The only small drawback I see that it looks odd if both the if- and else-branch do exactly the same thing, and it could invite people to open deduplication refactor PRs (or leave them wondering if it's a bug). Maybe adding some "this is
...

💬 theStack commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600351923)
nit, potential follow-up material: the test is imho a bit easier to follow if the naming refers to the original (non-malleated) counter-part:
```suggestion
tx_child_bad_wit = self.create_malleated_version(tx_child)
```
(same as for the other two sub-tests below)

💬 theuni commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2110709963)
Post-merge utACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d

💬 theuni commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2110711530)
@maflcko FWIW, I just suggested that c-i be hooked up so that we could see what real-world usage of the flags would be. I didn't mean to imply that it was the priority.

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600311972)
This is converting to feerate in sat/vb to (BTC/kvB)

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600381588)
Fixed, thanks

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600381746)
fixed

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600382055)
Yes, this is fixed

💬 theuni commented on pull request "build: Enable `thread_local` for MinGW-w64 builds":
(https://github.com/bitcoin/bitcoin/pull/30099#issuecomment-2110722669)
> The previously mentioned erroneous behavior is not an issue anymore

Erm, as of what version? Or what fix? We need more info here to be able to have any confidence.

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600383232)
Their is a description below that states that the feerate is in sat/vb so I will leave this as is.

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600383536)
Fixed

💬 ryanofsky commented on issue "build: `libbitcoin_util` unexpected(?)/undocumented dependencies":
(https://github.com/bitcoin/bitcoin/issues/28548#issuecomment-2110740202)
Looks like this issue should be resolved by #28548, which makes util no longer depend on common or consensus libraries. It does still depend on the crypto library, which #28548 handles by just documenting the dependency. Another option could be to move RNG and hashing functionality out of util so it does not depend on the crypto library anymore, but for now there does not seem to be a reason to do this.

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600400559)
Because `make_tx` is only constructing the transaction, the transaction created might not be confirmed, so Its inaccurate to add it to confirmed utxo's after constructing the transaction.

Transactions added to the confirmed utxo's in the diff (`sanity_check_rbf_estimates`) are all going to be confirmed eventually so we are accounting for them.

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-2110763673)
force push to cbc6c440e3:
- rename enum to `JSONRPCVersion::{V1_LEGACY, V2}`
- use `std::move()` in httprpc
- pass `catch_errors` argument to `JSONRPCExec()`
- replace `RPC_PARSE_ERROR` with `RPC_MISC_ERROR` in `JSONRPCExec()`

thanks again for the reviews @cbergqvist @ryanofsky

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600408347)
> does this also apply to other transactions in the block?

I don't fully understand your question.
The comment you highlighted refers to all incoming transactions into the mempool.

After a new block arrives, the fee estimator listens for notifications of all transactions removed from mempool because of the new block, and updates their tracking starts. All transactions with mempool dependencies that are mined are not tracked initially, they are just going to be skipped.

💬 theuni commented on pull request "rpc: move UniValue in blockToJSON":
(https://github.com/bitcoin/bitcoin/pull/30094#issuecomment-2110785523)
@willcl-ark Thanks!

Huh, I can't think of any reason why that would be the case. Saving on copies should be nothing but a win. Odd.

I think I'll start by opening a PR for the simplest/most obvious moves. Maybe you could help me bench those?

💬 AngusP commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#issuecomment-2110795746)
> Maybe adding some "this is intended" comment would make sense for a follow-up.

Could make the 'cast' explicit in the second branch, though there's already the comment making it pretty clear this is intentional so may be unnecessary

```c++
const auto guessed_wtxid = Wtxid::FromUint256(hash);
if (m_orphanage.HaveTx(guessed_wtxid)) return true;
```

🤔 sr-gi reviewed a pull request: "p2p: index TxOrphanage by wtxid, allow entries with same txid"
(https://github.com/bitcoin/bitcoin/pull/30000#pullrequestreview-2056028500)
crACK [0fb17bf](https://github.com/bitcoin/bitcoin/pull/30000/commits/0fb17bf61a40b73a2b81a18e70b3de180c917f22)

Non-blocking comments

💬 sr-gi commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600431768)
In b16da7eda76944719713be68b61f03d4acdd3e16

Is this the case? Didn't we prioritize children from the same peer, so given the parent was given by the honest, and he also has presented a child, we will pick that one?

https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net_processing.cpp#L3341-L3344

https://github.com/bitcoin/bitcoin/blob/dbb3113082a75035b14d20021036d2166171976e/src/net_processing.cpp#L3361-L3363

💬 sr-gi commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600417106)
In b16da7eda76944719713be68b61f03d4acdd3e16

The fake one is not really rejected, is it? It'll stay in the orphanage until a new block is mined (or until it times out)