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

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

✅ brunoerg closed a pull request: "test: fix check SENDTXRCNCL without WTXIDRELAY is ignored in `p2p_sendtxrcncl.py`"
(https://github.com/bitcoin/bitcoin/pull/30683)

💬 brunoerg commented on pull request "test: fix check SENDTXRCNCL without WTXIDRELAY is ignored in `p2p_sendtxrcncl.py`":
(https://github.com/bitcoin/bitcoin/pull/30683#issuecomment-2298981271)
https://github.com/bitcoin/bitcoin/pull/30683#discussion_r1723404448

💬 paplorinc commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#discussion_r1723413368)
My take was that `~/bitcoin/src` is easier to generalize to e.g. `C:\\Users\\Username\\Documents\\bitcoin\\src` than `/path/to/project/root/src` to `C:\\Users\\Username\\Documents\\bitcoin\\src`, since there have to understand that `root` means `bitcoin` and `src` refers to the literal `src` folder.

-----

My inspiration for using concrete examples was https://github.com/hebasto/bitcoin/pull/328/files#diff-4d2a64ce14cb8b971dbba9455421b04ae7ed0c489c66d983664be5632b0de4a3R19

-----

If it
...

💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#issuecomment-2299002666)
> I can confirm that best block locator and last_processed_block being different is confusing

I wonder if something else could be done to resolve this confusion other than writing to every wallet file every time a new block is connected, even if the block doesn't contain any relevant transactions. To me, "last block processed" and "last block flushed" seem like different concepts, and we could force them to be the same but only if we:

- Give up flexibility of not needing to flush each wall
...

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2299012568)
Force pushed to make the diff (minimally) smaller and easier to review. Should be trivial to re-review.