👍 theStack approved a pull request: "test: check xor.dat creation when missing"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2248062503)
ACK 7b7c287a33dc0c6f09bf32f31a8f05941b0cc9a2

Thanks for following up! If you want, you could also tackle the suggestion to move the `read_xor_key` helper to the `TestNode` class, see https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1717985952

💬 theStack commented on pull request "test: check xor.dat creation when missing":
(https://github.com/bitcoin/bitcoin/pull/30669#discussion_r1723350082)
nit, personally I prefer to be explicit about the zeroes initialization, but no big deal
```suggestion
NULL_BLK_XOR_KEY = bytes([0] * NUM_XOR_BYTES)
```

💬 theStack commented on pull request "test: add functional test for XORed block/undo files (`-blocksxor` option)":
(https://github.com/bitcoin/bitcoin/pull/30657#discussion_r1723358327)
> nit: Any reason to make this a global function and pass a node to it? Seems clearer to just make it a member function on the node, like `blocks_path()`, no?

No strong reason. Personally I first look into util.py for helpers and tend to forget that there are also some implemented as `TestNode` methods, but I agree that the latter makes sense. Could be picked up in a follow-up, e.g. #30669.

💬 glozow commented on pull request "test: fix check SENDTXRCNCL without WTXIDRELAY is ignored in `p2p_sendtxrcncl.py`":
(https://github.com/bitcoin/bitcoin/pull/30683#discussion_r1723364426)
Approach nack on using logs to test behavior. Is this covered in unit tests?

🤔 instagibbs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2245996944)
reviewed all refactorings through 388c2f5d7ee5f38cbefaff0c85cf298083127299

Pretty straight forward the whole way through.

going to spend time reviewing/testing the fuzz portions next

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1722086417)
3824d1f81ad9e4ff44ec46d07f4e8f86f27ce643 decent time to rename the function since that's been a previous discussion point?

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1722268907)
`maybe_add_extra_compact_tx` is starting to creep in usage vs its ostensible usage

I'm also not quire sure I parse this sentence. I presume it's trying to say this is only used in regular situations when freshly received over p2p.

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1722183824)
af3647050a85e6f33f902b903207043f6f0c023c preference for this to be inside own scope

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723352509)
CheckRequestsEmpty?

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1722276530)
d3fb8ca103b32f6071790bf7a9471fe4f80f5479

new ProcessInvalidTx return value needs to be documented

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723318351)
just noting this moved comment is extremely verbose/old comment and can probably just have the last paragraph kept?

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1722273831)
unrelated to your move-only change, but if txid==wtxid, looks like we're doing two insertions anyways here

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723349184)
If the wrapper is named `HaveMoreWork`, maybe have `GetTxToReconsider` be called `GetMoreWork`? Otherwise they seem incongruous.

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1723275394)
65d773feb7384a6893ee7e6a7b24dfa60f2d8d39

commit message seems grammatically incorrect:

"[refactor] move valid and tx processing to TxDownload"

💬 glozow commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1723369871)
Doesn't this just make the docs less generalizable? Why is that better?

💬 glozow commented on pull request "MiniWallet funcitonality for more readable code in functional test `rpc_signtransactionwithkey.py`":
(https://github.com/bitcoin/bitcoin/pull/30667#discussion_r1723372481)
The point of this test is to check that `signrawtransactionwithkey` works. Replacing the call to `signrawtransactionwithkey` with a different signing function seems wrong.

💬 ryanofsky commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-2298947289)
> rfm with 5 acks on the first commit (2 stale and 3 current), and 3 acks on the second commit?

I haven't been merging anything just because I think we are in feature freeze https://github.com/bitcoin/bitcoin/issues/29891

💬 brunoerg commented on pull request "test: fix check SENDTXRCNCL without WTXIDRELAY is ignored in `p2p_sendtxrcncl.py`":
(https://github.com/bitcoin/bitcoin/pull/30683#discussion_r1723380632)
> Is this covered in unit tests?

No, it doesn't cover any p2p iteration.

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1723392149)
Thanks, done. This is a good idea to reduce the diff and make it easier to review.

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1723397494)
Not sure. I understand that this will add compile-time checks, however I think the benefits in test code are limited, because:

* Tests are deterministic and execute fast (even in valgrind and other sanitizers), so any issues are found at roughly the same CPU cost.
* If I change it here, I open up the door to change it elsewhere in this diff, which will make it more verbose and harder to review.
* The resulting code is more verbose at little benefit

So I'll leave this as-is for now.