💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205033216)
6ce95bffee0f809d9bf4d8fed6f8a7df077fa83f

I think txorphanage_sim subsumes the older `txorphan` harness now unless I'm missing something

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205019742)
do we want to call this after every `AddTx` above instead of after adding all additional announcers?

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205060872)
```Suggestion
assert(m_orphans.size() >= m_unique_orphans);
assert(m_orphans.size() <= m_peer_orphanage_info.size() * m_unique_orphans);
```

💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205053496)
d5097d6eef85eab971352cf78fb94a30c6e6f127

looks like you forgot to assert the calculated set matches anything ala

assert(m_peer_orphanage_info == reconstructed_peer_info);

💬 stringintech commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#issuecomment-3069805059)
[1154618](https://github.com/bitcoin/bitcoin/commit/11546183c70def6c0aa539642fd1c9ada3d46840) to [61e800e](https://github.com/bitcoin/bitcoin/commit/61e800e75cffa256ccdbc2ffc7a1739c00880ce0) - Applied suggestion by @stratospher to move helper methods before their respective subtest. Thanks!

💬 stringintech commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2205078542)
While I personally prefer a top-down approach where the main test methods are visible at the top without much scrolling, I am seeing the pattern you described is mostly respected across the tests, so I applied it.

💬 maflcko commented on pull request "test: fix intermittent failure in rpc_invalidateblock.py":
(https://github.com/bitcoin/bitcoin/pull/32968#discussion_r2205112609)

“Since header doesn't have block data, it can't be chain tip” -> “Since the header doesn't have block data, it can't be the chain tip” [missing definite articles]

💬 maflcko commented on pull request "test: fix intermittent failure in rpc_invalidateblock.py":
(https://github.com/bitcoin/bitcoin/pull/32968#issuecomment-3069863625)
lgtm ACK 28416f367a5d720d89214aa8ef94013caa1d1a40

Can be tested via by moving the lines after ` self.nodes[0].reconsiderblock(block.hash)` down again and observing a failure.

👋 ishaanam's pull request is ready for review: "wallet, rpc: add v3 transaction creation and wallet support"
(https://github.com/bitcoin/bitcoin/pull/32896)

💬 ishaanam commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#issuecomment-3069883620)
This PR is now ready for review.

🤔 sipa reviewed a pull request: "cluster mempool: add TxGraph work controls"
(https://github.com/bitcoin/bitcoin/pull/32263#pullrequestreview-3016660633)
Addressed comments, and also made some further simplifications (dropped the returning of amount of work performed by `MakeAcceptable` and `MakeAllAcceptable` in the preparation commit, as the new `DoWork()` doesn't use them anymore but invokes `Cluster::Relinearize` directly).

💬 sipa commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2205126924)
I have added some additional comments.

💬 sipa commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2205127512)
Done.

💬 sipa commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2205127983)
Added some comments to clarify.

✅ maflcko closed a pull request: "rpc: Support v3 raw transactions creation"
(https://github.com/bitcoin/bitcoin/pull/31936)

💬 maflcko commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#issuecomment-3069899126)
Closing for now, due to inactivity. The given feedback hasn't bee addressed over a long time and at this point it may be best to focus review on https://github.com/bitcoin/bitcoin/pull/32896.

💬 josibake commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3069964449)



> > am not sure how I am to interpret your NACK here.
>
> Just as "I think this PR should not be merged in its current form." I definitely do agree with the approach of adding a C API.

FWIW, I read this as "Concept ACK, Approach NACK" (per https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#conceptual-review), which I think is a helpful distinction.

💬 maflcko commented on pull request "refactor: cleanup index logging":
(https://github.com/bitcoin/bitcoin/pull/32948#issuecomment-3069982232)
review ACK c18bf0bd9be632e54591493a5309e9ed4020f2e5 🚣

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK c18bf0bd9be6
...

💬 purpleKarrot commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3069994277)
Yes, Concept ACK, Approach NACK

Thanks, @josibake.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3070013951)
Updated 267a7b3f321304f75e8c47e380da49ba9c64bc84 -> 1ffc1c9d94b16cdbfb92a26d0f0e75451efad4fe ([kernelApi_44](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_44) -> [kernelApi_45](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_45), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_44..kernelApi_45))

* Enforce better function names in the API, which should make future discussions on their desired end format a bit easier.
* Dropped the macro check for gcc 4.