💬 mzumsande commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-1928027472)
[87d6155 ](https://github.com/bitcoin/bitcoin/commit/87d6155b91797d795433c9c8bca5da20c74f810c)to [efdc8e3](https://github.com/bitcoin/bitcoin/commit/efdc8e3a0ee81b143ee17a578f1e27b1c427a912): rebased

🤔 sr-gi reviewed a pull request: "test: p2p: adhere to typical VERSION message protocol flow"
(https://github.com/bitcoin/bitcoin/pull/29353#pullrequestreview-1854629637)
tACK utACK https://github.com/bitcoin/bitcoin/commit/c340503b67585b631909584f1acaa327f5af25d3

As a side note, I found the newly added comments regarding inbounds and outbounds to be a bit confusing. This is due to them being written from the POV of the Python node instead of the TestNode's, which is the logic used by `add_p2p_connection` and `add_outbound_p2p_connection`

💬 sr-gi commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#discussion_r1473340868)
This is really just calling `super().send_version`. We could get rid of the whole class given no other method is being overwritten (or leave the alias if we want it for readability, but get rid of the method)

💬 sr-gi commented on pull request "test: p2p: adhere to typical VERSION message protocol flow":
(https://github.com/bitcoin/bitcoin/pull/29353#discussion_r1473341794)
Same as for `p2p_add_connections::P2PFeelerReceiver`

💬 edilmedeiros commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478875833)
I see that you don't change the hardcoded value `0xc0`, just pass it with function calls.
It does make sense to generalize `taproot_tree_helper` and `taproot_construct`.

💬 edilmedeiros commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478878383)
I'm not familiar with taproot, but do you think it does make sense to add a test to check if someone is passing the correct version constants to the functions?

📝 brunoerg opened a pull request: "fuzz: remove unused `args` and `context` from `FuzzedWallet`"
(https://github.com/bitcoin/bitcoin/pull/29388)
`ArgsManager args` and `WalletContext context` were previously used to create the wallet into `FuzzedWallet`. After fa15861763df71e788849b587883b3c16bb12229, they are not used anymore. This PR removes them.

💬 theuni commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1928047502)
@sipa Huh, thanks.

I could've sworn I saw an autoconf check for that that defined it when I was grepping around last week, but I guess I got it mixed up in my head with another. The lack of `HAVE_` or `USE_` should've been a giveaway :)

💬 brunoerg commented on pull request "fuzz: remove unused `args` and `context` from `FuzzedWallet`":
(https://github.com/bitcoin/bitcoin/pull/29388#issuecomment-1928050767)
cc: @maflcko

💬 theuni commented on issue "doc: List identifiers used from header in #if(def)":
(https://github.com/bitcoin/bitcoin/issues/26972#issuecomment-1928054768)
Aha, it used to be an autoconf check: #23761

That explains the zombie include.

💬 Christewart commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478936428)
I think you probably need to understand Taproot at least a bit to understand why we aren't changing the hard coded constant `0xc0` for now. I would recommend taking a look at #29221 to understand what a new using a _new_ leaf version looks like.

💬 furszy commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#discussion_r1478916123)
This is an extreme edge case, but unlocking the semaphore prior to refreshing the connection could theoretically allow another thread to dump this information to disk. To be 100% sure that no race will ever happen, this line should be after the connection refreshing process (after line 447).

🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1863823000)
Code reviewed, looking good. Left a last comment.

Also, have slightly modified my previous unit test to validate the changes (ba333a239b8aec4a7a6a79d11d4668730ee24803).
It is not really needed but feel free to take it if you like it (conceptually, it is similar to your test, the diff is that it is verifying the writes execution order and checks that when a batch finishes writing data to disk, another batch in a concurrent process can read the data immediately).

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478943273)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477981888

Thanks, restored this line

🤔 ryanofsky reviewed a pull request: "assumeutxo: Get rid of faked nTx and nChainTx values"
(https://github.com/bitcoin/bitcoin/pull/29370#pullrequestreview-1863598337)
Updated a3228f02f69e28960f7da76054965c916e781f27 -> 3bdbc3fc7f116856acc7d955f591110b96f5ebd2 ([`pr/nofake.2`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.2) -> [`pr/nofake.3`](https://github.com/ryanofsky/bitcoin/commits/pr/nofake.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/nofake.2..pr/nofake.3)) implementing all suggestions, adding test coverage for the RPCs, and adding a new commit that drops the no longer needed BLOCK_ASSUMED_VALID flag.

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478766159)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477996772

Thanks, removed this

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478757569)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998490

Added a comment. It is necessary to set nSequenceId to avoid an assert failure in CheckBlockIndex. I think it's also a reasonable thing to do to make the block look like it was never received.

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478776687)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478006900

Thanks, reverted this

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478947294)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478022794

Restored this check

💬 ryanofsky commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1478766291)
re: https://github.com/bitcoin/bitcoin/pull/29370#discussion_r1477998941

> Same (sequence id)

Thanks, removed this