💬 instagibbs commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2204933624)
It will not "expend" iterations on oversized levels, so it can return "true" when doing no work.

💬 instagibbs commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2204876202)
s/quality/QualityLevel/

💬 instagibbs commented on pull request "cluster mempool: add TxGraph work controls":
(https://github.com/bitcoin/bitcoin/pull/32263#discussion_r2204911282)
The new logic means that you may not have enough to make it considered `ACCEPTABLE` BUT you may end up with `OPTIMAL` anyways and are able to continue until you hit a difficult cluster. In practice this may end up with a lot more clusters known as optimal when their topology is simple.

Might be worth a comment on the "if (!improved) return false;" line

💬 maflcko commented on issue "ci: don't pass GIT_EXEC_PATH inside test container":
(https://github.com/bitcoin/bitcoin/issues/32935#issuecomment-3069728608)
Maybe an easy fix could be to not pass any env vars, except those with a key specified in `ci/test/00_setup_env.sh`?

🤔 instagibbs reviewed a pull request: "p2p: improve TxOrphanage denial of service bounds"
(https://github.com/bitcoin/bitcoin/pull/31829#pullrequestreview-3016494838)
reviewed through 790f6e7a72d31988c5be9f2d520a04d0c0edfa09

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