🤔 furszy reviewed a pull request: "index: fix wrong assert of current_tip == m_best_block_index"
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-3022570828)
> General question - should we remove the assert, or would it maybe be better to keep it, but commit the best block index before calling `Rewind()` in `Sync()`?
I'm not sure about this but.. if we go down that road, wouldn't it be simpler to just drop the `current_tip` arg and use `m_best_block_index` internally?
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-3022570828)
> General question - should we remove the assert, or would it maybe be better to keep it, but commit the best block index before calling `Rewind()` in `Sync()`?
I'm not sure about this but.. if we go down that road, wouldn't it be simpler to just drop the `current_tip` arg and use `m_best_block_index` internally?
💬 waketraindev commented on pull request "rpc: add optional peer_ids param to filter getpeerinfo":
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3076011613)
> I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
Skipped type checking and added simple cases so `getpeerinfo 7` and `getpeerinfo '[1,2,3]'` should work
(https://github.com/bitcoin/bitcoin/pull/32741#issuecomment-3076011613)
> I would do peer_id|peer_ids and accept both a single number (most common use) or an array (efficient).
Skipped type checking and added simple cases so `getpeerinfo 7` and `getpeerinfo '[1,2,3]'` should work
💬 kurapika007 commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3076029783)
> Reducing `DEFAULT_MIN_RELAY_TX_FEE` only makes sense to me in conjunction with reducing `DEFAULT_BLOCK_MIN_TX_FEE`. Otherwise nodes become subject to free relay attacks that could be used to waste bandwidth across the network with transactions that are not even in danger of getting mined in most blocks.
>
> > `DEFAULT_INCREMENTAL_RELAY_FEE` should be decreased along with this as well.
>
> That’s probably a bad idea, as it would increase the surface for bandwidth wasting attacks 10× even
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3076029783)
> Reducing `DEFAULT_MIN_RELAY_TX_FEE` only makes sense to me in conjunction with reducing `DEFAULT_BLOCK_MIN_TX_FEE`. Otherwise nodes become subject to free relay attacks that could be used to waste bandwidth across the network with transactions that are not even in danger of getting mined in most blocks.
>
> > `DEFAULT_INCREMENTAL_RELAY_FEE` should be decreased along with this as well.
>
> That’s probably a bad idea, as it would increase the surface for bandwidth wasting attacks 10× even
...
💬 PJJacobowitz commented on issue "ARM Windows build and release":
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3076030848)
Thank you @maflcko . Would you know how to get this into the mainline and then publish it so Windows on ARM users can download it via the link below?
https://bitcoin.org/en/download
(https://github.com/bitcoin/bitcoin/issues/31388#issuecomment-3076030848)
Thank you @maflcko . Would you know how to get this into the mainline and then publish it so Windows on ARM users can download it via the link below?
https://bitcoin.org/en/download
📝 stickies-v opened a pull request: "rpc: refactor: use string_view in Arg/MaybeArg"
(https://github.com/bitcoin/bitcoin/pull/32983)
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](https://github.com/bitcoin/bitcoin/pull/32845/commits/d127b25199118756122232d9aafff19f1922869b#diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a3237
...
(https://github.com/bitcoin/bitcoin/pull/32983)
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](https://github.com/bitcoin/bitcoin/pull/32845/commits/d127b25199118756122232d9aafff19f1922869b#diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a3237
...
📝 achow101 opened a pull request: "wallet: Set migrated wallet name only on success"
(https://github.com/bitcoin/bitcoin/pull/32984)
After a wallet is migrated and we are trying to load it, if it could not be loaded, don't try to set the wallet name. Otherwise we have a segfault.
This can be tested by migrated a legacy wallet from another network (e.g. trying to migrate a testnet wallet on mainnet). The fixed behavior is return an error and restore the backup.
I was unable to write a working functional test for this behavior.
(https://github.com/bitcoin/bitcoin/pull/32984)
After a wallet is migrated and we are trying to load it, if it could not be loaded, don't try to set the wallet name. Otherwise we have a segfault.
This can be tested by migrated a legacy wallet from another network (e.g. trying to migrate a testnet wallet on mainnet). The fixed behavior is return an error and restore the backup.
I was unable to write a working functional test for this behavior.
📝 achow101 opened a pull request: "wallet: Always rewrite tx records during migration"
(https://github.com/bitcoin/bitcoin/pull/32985)
Since loading a wallet may change some parts of tx records (e.g. adding nOrderPos), we should rewrite the records instead of copying them so that the automatic upgrade does not need to be performed again when the wallet is loaded.
This is useful for future PRs I'm working on where we can be sure about what data exists in a tx record in descriptor wallets.
(https://github.com/bitcoin/bitcoin/pull/32985)
Since loading a wallet may change some parts of tx records (e.g. adding nOrderPos), we should rewrite the records instead of copying them so that the automatic upgrade does not need to be performed again when the wallet is loaded.
This is useful for future PRs I'm working on where we can be sure about what data exists in a tx record in descriptor wallets.
🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3022449462)
Thanks for the comments, [addressed them](https://github.com/bitcoin/bitcoin/compare/26e998986af5fcdb235d1551172f1062bf30309a..4a67b26aeda4dc98fc008091743a7cc3bf6ec013) + rebased in a separate push.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3022449462)
Thanks for the comments, [addressed them](https://github.com/bitcoin/bitcoin/compare/26e998986af5fcdb235d1551172f1062bf30309a..4a67b26aeda4dc98fc008091743a7cc3bf6ec013) + rebased in a separate push.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2208775351)
Thanks, moved
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2208775351)
Thanks, moved
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2208831588)
I have extracted it to a new commit at the beginning which hard-codes the size and uses is throughout the codebase, so we get to delete `OBFUSCATION_SIZE_BYTES` early, so the rename scripted diff doen't need it anymore.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2208831588)
I have extracted it to a new commit at the beginning which hard-codes the size and uses is throughout the codebase, so we get to delete `OBFUSCATION_SIZE_BYTES` early, so the rename scripted diff doen't need it anymore.
💬 l0rinc commented on pull request "[IBD] multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2208857562)
> I still don't understand why the same line is changed/renamed several times
Because they usually reflect the name that I found to best describe their role in the current commit.
I have changed these to reflect the final state instead so we shouldn't have back-and-forth renames anymore (remaining ones are are deliberate, e.g. we need both the underlying bytes and the `Obfuscation` object).
> let the very first commit be [...] class Obfuscation [...] and then use that from the beginning
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2208857562)
> I still don't understand why the same line is changed/renamed several times
Because they usually reflect the name that I found to best describe their role in the current commit.
I have changed these to reflect the final state instead so we shouldn't have back-and-forth renames anymore (remaining ones are are deliberate, e.g. we need both the underlying bytes and the `Obfuscation` object).
> let the very first commit be [...] class Obfuscation [...] and then use that from the beginning
...
🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3022666436)
Thanks for the comments, [addressed them](https://github.com/bitcoin/bitcoin/compare/26e998986af5fcdb235d1551172f1062bf30309a..4a67b26aeda4dc98fc008091743a7cc3bf6ec013) + rebased in a separate push.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3022666436)
Thanks for the comments, [addressed them](https://github.com/bitcoin/bitcoin/compare/26e998986af5fcdb235d1551172f1062bf30309a..4a67b26aeda4dc98fc008091743a7cc3bf6ec013) + rebased in a separate push.
🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3022666508)
Thanks for the comments, [addressed them](https://github.com/bitcoin/bitcoin/compare/26e998986af5fcdb235d1551172f1062bf30309a..4a67b26aeda4dc98fc008091743a7cc3bf6ec013) + rebased in a separate push.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3022666508)
Thanks for the comments, [addressed them](https://github.com/bitcoin/bitcoin/compare/26e998986af5fcdb235d1551172f1062bf30309a..4a67b26aeda4dc98fc008091743a7cc3bf6ec013) + rebased in a separate push.
🤔 l0rinc reviewed a pull request: "[IBD] multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3022666542)
Thanks for the comments, [addressed them](https://github.com/bitcoin/bitcoin/compare/26e998986af5fcdb235d1551172f1062bf30309a..4a67b26aeda4dc98fc008091743a7cc3bf6ec013) + rebased in a separate push.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-3022666542)
Thanks for the comments, [addressed them](https://github.com/bitcoin/bitcoin/compare/26e998986af5fcdb235d1551172f1062bf30309a..4a67b26aeda4dc98fc008091743a7cc3bf6ec013) + rebased in a separate push.
💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208503121)
nit: parameter names are defined per RPC, so assuming it's called "wallet_name" is not ideal, even if at the moment it seems that the assumption is correct for all RPCs (as far as I can see). I don't have a better solution yet, but I'll think about it more.
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208503121)
nit: parameter names are defined per RPC, so assuming it's called "wallet_name" is not ideal, even if at the moment it seems that the assumption is correct for all RPCs (as far as I can see). I don't have a better solution yet, but I'll think about it more.
💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2204543796)
It seems like our usual way to test this is to use the RPC name as the search text. I also don't think we need to be overly verbose in logging every single statement. Could simplify this test to:
```py
def test_required_args(self, node):
self.log.info("Test that required arguments must be passed")
assert_raises_rpc_error(-1, "getdescriptoractivity", node.getdescriptoractivity)
assert_raises_rpc_error(-1, "getdescriptoractivity", node.getdescriptoractivity, [])
...
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2204543796)
It seems like our usual way to test this is to use the RPC name as the search text. I also don't think we need to be overly verbose in logging every single statement. Could simplify this test to:
```py
def test_required_args(self, node):
self.log.info("Test that required arguments must be passed")
assert_raises_rpc_error(-1, "getdescriptoractivity", node.getdescriptoractivity)
assert_raises_rpc_error(-1, "getdescriptoractivity", node.getdescriptoractivity, [])
...
💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2204560989)
I don't really see the benefit of replacing these brief and easy-to-read definitions with a variable that needs to be inspected out-of-line, especially since descriptions may need to be tailored to the specific RPC functionality. Would prefer keeping those 2 as-is?
Also nit: I find `output_descriptor_obj` a better name than `obj_with_output_descriptor`
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2204560989)
I don't really see the benefit of replacing these brief and easy-to-read definitions with a variable that needs to be inspected out-of-line, especially since descriptions may need to be tailored to the specific RPC functionality. Would prefer keeping those 2 as-is?
Also nit: I find `output_descriptor_obj` a better name than `obj_with_output_descriptor`
💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208893832)
I think we can avoid using raw pointers and move to a more idiomatic interface by updating the arg helpers to use string_view instead of string, making it trivially copyable. I've implemented that in #32983. Shouldn't block progress - but if it gets merged first, it might be worth adopting that interface here too.
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208893832)
I think we can avoid using raw pointers and move to a more idiomatic interface by updating the arg helpers to use string_view instead of string, making it trivially copyable. I've implemented that in #32983. Shouldn't block progress - but if it gets merged first, it might be worth adopting that interface here too.
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208955963)
> Would prefer keeping those 2 as-is?
Ok, I did it that way just for reuse purpose, to avoid having a typo on the strings and so on.
`obj_with_output_descriptor` comes from the definition: "An object with output descriptor and metadata", but I'm ok with `output_descriptor_obj`.
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208955963)
> Would prefer keeping those 2 as-is?
Ok, I did it that way just for reuse purpose, to avoid having a typo on the strings and so on.
`obj_with_output_descriptor` comes from the definition: "An object with output descriptor and metadata", but I'm ok with `output_descriptor_obj`.
💬 pablomartin4btc commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208959369)
I thought to use `<std::string_view>` too when I did the `TMPL_INST` but didn't go forward. We can keep it this way if this gets merged first or if it does yours I can incorporate it here yeah. I'll try to review your PR tmw. Thanks!
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2208959369)
I thought to use `<std::string_view>` too when I did the `TMPL_INST` but didn't go forward. We can keep it this way if this gets merged first or if it does yours I can incorporate it here yeah. I'll try to review your PR tmw. Thanks!