💬 Christewart commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478815788)
here is where the previous hard coded value was that the parameter is replacing.
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1478815788)
here is where the previous hard coded value was that the parameter is replacing.
💬 mzumsande commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1927937515)
Will address feedback and https://github.com/bitcoin/bitcoin/pull/29356#discussion_r1478769478 after the merge of #29356
(https://github.com/bitcoin/bitcoin/pull/29358#issuecomment-1927937515)
Will address feedback and https://github.com/bitcoin/bitcoin/pull/29356#discussion_r1478769478 after the merge of #29356
📝 mzumsande converted_to_draft a pull request: "test: use v2 everywhere for P2PConnection if --v2transport is enabled"
(https://github.com/bitcoin/bitcoin/pull/29358)
#24748 added v2 transport to the python `P2PConnection`, but so far each test that wants to make use of it needs to enable it on an individual basis.
This PR changes it so that if the test suite is run with `--v2transport` option, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also wherever `P2PConnection` is used. Individual tests can override this global option.
To do that, a few tests need to be adjusted, which is done in the fir
...
(https://github.com/bitcoin/bitcoin/pull/29358)
#24748 added v2 transport to the python `P2PConnection`, but so far each test that wants to make use of it needs to enable it on an individual basis.
This PR changes it so that if the test suite is run with `--v2transport` option, v2 is used in each test by default, not only for connections between two bitcoind instances as before, but also wherever `P2PConnection` is used. Individual tests can override this global option.
To do that, a few tests need to be adjusted, which is done in the fir
...
💬 tcharding commented on issue "`libbitcoinconsensus.a` is unusable":
(https://github.com/bitcoin/bitcoin/issues/28779#issuecomment-1928003536)
Thanks for considering us.
(https://github.com/bitcoin/bitcoin/issues/28779#issuecomment-1928003536)
Thanks for considering us.
💬 mzumsande commented on pull request "p2p: Increase inbound capacity for block-relay only connections":
(https://github.com/bitcoin/bitcoin/pull/28463#issuecomment-1928020517)
[a9a64f9 ](https://github.com/bitcoin/bitcoin/commit/a9a64f990e1029ed89d2d08e76b96dcba6e8e91e)to [77a612d](https://github.com/bitcoin/bitcoin/commit/77a612dd75eb2383d62d2d21c182684ff2026416): rebased
(https://github.com/bitcoin/bitcoin/pull/28463#issuecomment-1928020517)
[a9a64f9 ](https://github.com/bitcoin/bitcoin/commit/a9a64f990e1029ed89d2d08e76b96dcba6e8e91e)to [77a612d](https://github.com/bitcoin/bitcoin/commit/77a612dd75eb2383d62d2d21c182684ff2026416): rebased
💬 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
(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`
(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)
(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`
(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`.
(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?
(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.
(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 :)
(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
(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.
(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.
(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).
(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).
(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
(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.
(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.