💬 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)
```
(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.
(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?
(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
(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?
(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.
(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
(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?
(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
(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?
(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
(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.
(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"
(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?
(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.
(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
(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.
(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.
(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.
(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.
💬 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_r1723404448)
Tbh, most part of the functional test relies on checking logs which I agree it's a bad way to test behavior. However, since we're just registering and forgetting peers, there is no other way to test stuff on the functional side, except disconnections.
I'll close this for now and suggest some improvements for this test according to the Erlay PRs.
(https://github.com/bitcoin/bitcoin/pull/30683#discussion_r1723404448)
Tbh, most part of the functional test relies on checking logs which I agree it's a bad way to test behavior. However, since we're just registering and forgetting peers, there is no other way to test stuff on the functional side, except disconnections.
I'll close this for now and suggest some improvements for this test according to the Erlay PRs.