💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1973895223)
Thanks, great suggestion.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1973895223)
Thanks, great suggestion.
💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1973873512)
Okay, I renamed it to "HaveEquivalentValues"
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1973873512)
Okay, I renamed it to "HaveEquivalentValues"
💬 kevkevinpal commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#issuecomment-2688654353)
ACK 6109bc529e234b464d406495bb9644254ca7fc9e
(https://github.com/bitcoin/bitcoin/pull/31823#issuecomment-2688654353)
ACK 6109bc529e234b464d406495bb9644254ca7fc9e
💬 kevkevinpal commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#issuecomment-2688673600)
The CI failure looks unrelated to your change
(https://github.com/bitcoin/bitcoin/pull/31823#issuecomment-2688673600)
The CI failure looks unrelated to your change
💬 pablomartin4btc commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1974100156)
Ok, I had 18.1.8, updated it to 19, rerun the merge and I can confirm that the "counter mismatch" has gone. nit: Please consider adding a note there regarding the message and the warning with the 2 functions mismatch, any of those didn't affect the generation of the report nor its visualisation. Thanks!
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1974100156)
Ok, I had 18.1.8, updated it to 19, rerun the merge and I can confirm that the "counter mismatch" has gone. nit: Please consider adding a note there regarding the message and the warning with the 2 functions mismatch, any of those didn't affect the generation of the report nor its visualisation. Thanks!
💬 Christewart commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#issuecomment-2688724966)
> The CI failure looks unrelated to your change
Maybe the gods (AKA bitcoin core maintainers 👼 📿 ) will restart the build for me :pray: 😄
(https://github.com/bitcoin/bitcoin/pull/31823#issuecomment-2688724966)
> The CI failure looks unrelated to your change
Maybe the gods (AKA bitcoin core maintainers 👼 📿 ) will restart the build for me :pray: 😄
📝 jonatack opened a pull request: "seeds: update makeseeds regex and DNS fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/31960)
for the upcoming release per the instructions in `/contrib/seeds/README.md`.
In `makeseeds.py` the user agent regex needed updating (along with the I2P column header spacing).
The previous 2 seeds updates were https://github.com/bitcoin/bitcoin/pull/30008 and https://github.com/bitcoin/bitcoin/pull/30695.
```
/contrib/seeds$ python3 makeseeds.py -a asmap-filled.dat -s seeds_main.txt > nodes_main.txt
Loading asmap database "asmap-filled.dat"…Done.
Loading and parsing DNS seeds…Done.
...
(https://github.com/bitcoin/bitcoin/pull/31960)
for the upcoming release per the instructions in `/contrib/seeds/README.md`.
In `makeseeds.py` the user agent regex needed updating (along with the I2P column header spacing).
The previous 2 seeds updates were https://github.com/bitcoin/bitcoin/pull/30008 and https://github.com/bitcoin/bitcoin/pull/30695.
```
/contrib/seeds$ python3 makeseeds.py -a asmap-filled.dat -s seeds_main.txt > nodes_main.txt
Loading asmap database "asmap-filled.dat"…Done.
Loading and parsing DNS seeds…Done.
...
💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1974177716)
Update from offline conversation: This was a confusion about what we're calling "optimal". The splits clusters are already optimal in the sense of the diagram check, but not minimal or fully connected. PostLinearization makes sure that all chunks are connected/minimal.
Perhaps a rewording of this section to make it abundantly clear?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1974177716)
Update from offline conversation: This was a confusion about what we're calling "optimal". The splits clusters are already optimal in the sense of the diagram check, but not minimal or fully connected. PostLinearization makes sure that all chunks are connected/minimal.
Perhaps a rewording of this section to make it abundantly clear?
✅ instagibbs closed a pull request: "test: add coverage for immediate orphanage eviction case"
(https://github.com/bitcoin/bitcoin/pull/31628)
(https://github.com/bitcoin/bitcoin/pull/31628)
💬 instagibbs commented on pull request "test: add coverage for immediate orphanage eviction case":
(https://github.com/bitcoin/bitcoin/pull/31628#issuecomment-2688855909)
I'm not convinced this is the right approach in the end. As @glozow mentions this isn't testing desired behavior, and the behavior will likely dramatically change soon, requiring ripping out much of the test.
I think it's better to use fuzzers to get coverage of this and check the invariants like "don't crash".
(https://github.com/bitcoin/bitcoin/pull/31628#issuecomment-2688855909)
I'm not convinced this is the right approach in the end. As @glozow mentions this isn't testing desired behavior, and the behavior will likely dramatically change soon, requiring ripping out much of the test.
I think it's better to use fuzzers to get coverage of this and check the invariants like "don't crash".
💬 murchandamus commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1974198164)
The RPC is "sendmany" without the underscore.
```suggestion
- The `settxfee` RPC is now **deprecated** and will be removed in Bitcoin Core 31.0. This RPC allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can lead to overpaying or underpaying. Users should instead rely on fee estimation or specify a fee rate per transaction using the `fee_rate` argument in RPCs like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, an
...
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1974198164)
The RPC is "sendmany" without the underscore.
```suggestion
- The `settxfee` RPC is now **deprecated** and will be removed in Bitcoin Core 31.0. This RPC allowed users to set a static fee rate for wallet transactions, which is discouraged in a dynamic fee environment as it can lead to overpaying or underpaying. Users should instead rely on fee estimation or specify a fee rate per transaction using the `fee_rate` argument in RPCs like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, an
...
💬 murchandamus commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1974199354)
Same:
```suggestion
- The `-paytxfee` setting is now **deprecated** and will be removed in Bitcoin Core 31.0. This option set a static fee rate for all wallet transactions when starting the Bitcoin node, which is discouraged in a dynamic fee environment as it can lead to overpaying or underpaying. Users should instead rely on fee estimation or specify a fee rate per transaction using the `fee_rate` argument in RPCs like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `sendmany`.
...
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1974199354)
Same:
```suggestion
- The `-paytxfee` setting is now **deprecated** and will be removed in Bitcoin Core 31.0. This option set a static fee rate for all wallet transactions when starting the Bitcoin node, which is discouraged in a dynamic fee environment as it can lead to overpaying or underpaying. Users should instead rely on fee estimation or specify a fee rate per transaction using the `fee_rate` argument in RPCs like `fundrawtransaction`, `sendtoaddress`, `send`, `sendall`, and `sendmany`.
...
💬 murchandamus commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1974201320)
It seems to me that you agreed with @fjahr on his comment here, but the RPC is still being hidden. Is that an oversight?
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1974201320)
It seems to me that you agreed with @fjahr on his comment here, but the RPC is still being hidden. Is that an oversight?
💬 i-am-yuvi commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#issuecomment-2688892825)
Approach ACK
Correct me if I misunderstood:
In Python, when a function accepts both positional(*args) and keyword(**argsn) arguments, for which args is handled as a tuple and argsn as dictionary where Python doesn't merge them automatically into a combined list.
## The issue
The RPC server accepts a single call which includes both positional as well as keyword. That means if a caller provides both, the server expects to receive both types together. In that case, the original code(logg
...
(https://github.com/bitcoin/bitcoin/pull/31955#issuecomment-2688892825)
Approach ACK
Correct me if I misunderstood:
In Python, when a function accepts both positional(*args) and keyword(**argsn) arguments, for which args is handled as a tuple and argsn as dictionary where Python doesn't merge them automatically into a combined list.
## The issue
The RPC server accepts a single call which includes both positional as well as keyword. That means if a caller provides both, the server expects to receive both types together. In that case, the original code(logg
...
🤔 i-am-yuvi reviewed a pull request: "test: Fix authproxy named args debug logging"
(https://github.com/bitcoin/bitcoin/pull/31955#pullrequestreview-2648856215)
Tested ACK fac1dd9dffba1033245c283bc0468e801c14e910
I could see the difference:
`-1-> getblockcount {}` - without changes
`-1-> getblockcount [] {}` - with changes
(https://github.com/bitcoin/bitcoin/pull/31955#pullrequestreview-2648856215)
Tested ACK fac1dd9dffba1033245c283bc0468e801c14e910
I could see the difference:
`-1-> getblockcount {}` - without changes
`-1-> getblockcount [] {}` - with changes
💬 i-am-yuvi commented on pull request "test: Fix authproxy named args debug logging":
(https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1974209231)
This helper function makes sense that instead of using repetitive code which might cause inconsistencies this will wrap *json.dumps* so that every part of code uses uniform serialization.
(https://github.com/bitcoin/bitcoin/pull/31955#discussion_r1974209231)
This helper function makes sense that instead of using repetitive code which might cause inconsistencies this will wrap *json.dumps* so that every part of code uses uniform serialization.
💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2688952065)
At first glance it makes sense to make 189eb8c02b5b81fe3239df34cd9ef48871e2e5de its own PR, i.e. making sqlite mandatory for wallet building. See https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1963117438
(https://github.com/bitcoin/bitcoin/pull/31250#issuecomment-2688952065)
At first glance it makes sense to make 189eb8c02b5b81fe3239df34cd9ef48871e2e5de its own PR, i.e. making sqlite mandatory for wallet building. See https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1963117438
🤔 BrandonOdiwuor reviewed a pull request: "doc: Update documentation to include Clang/llvm based coverage report generation"
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2649006319)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31933#pullrequestreview-2649006319)
Concept ACK
📝 Sjors opened a pull request: "Require sqlite when building the wallet"
(https://github.com/bitcoin/bitcoin/pull/31961)
Require that sqlite is available in order to compile the wallet. Removes instances of `USE_SQLITE` since it is no longer possible to not have sqlite available.
Extracted from #31250
(https://github.com/bitcoin/bitcoin/pull/31961)
Require that sqlite is available in order to compile the wallet. Removes instances of `USE_SQLITE` since it is no longer possible to not have sqlite available.
Extracted from #31250
💬 Sjors commented on pull request "wallet: Disable creating and loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1974296746)
Done in #31961
(https://github.com/bitcoin/bitcoin/pull/31250#discussion_r1974296746)
Done in #31961