👍 ryanofsky approved a pull request: "assumeutxo: Add dumptxoutset height param, remove shell scripts"
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2132676568)
Code review ACK 6a5408dd7da4c31283cb4f632b3a7aa692a53ca3. This is a really nice improvement over existing shell scripts, with a surprisingly simple implementation. I left some suggestions, but none are important and this looks good to merge in its current form.
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2132676568)
Code review ACK 6a5408dd7da4c31283cb4f632b3a7aa692a53ca3. This is a really nice improvement over existing shell scripts, with a surprisingly simple implementation. I left some suggestions, but none are important and this looks good to merge in its current form.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1648974658)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
I don't think it's sufficient to call `connman.SetNetworkActive()` manually in this function, because the InvalidateBlock and ReconsiderBlock functions called here can both throw exceptions and leave the network disabled after this RPC call if there is an error. Would suggest making a NetworkDisable disable_network{connman}; RAII class that disables the network in its constructor and enables it in
...
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1648974658)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
I don't think it's sufficient to call `connman.SetNetworkActive()` manually in this function, because the InvalidateBlock and ReconsiderBlock functions called here can both throw exceptions and leave the network disabled after this RPC call if there is an error. Would suggest making a NetworkDisable disable_network{connman}; RAII class that disables the network in its constructor and enables it in
...
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649320665)
In commit "doc: Improve guide for usage of assumeutxo" (6a5408dd7da4c31283cb4f632b3a7aa692a53ca3)
Maybe say "generate it yourself from another node" to be clear you need a different node to generate it.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649320665)
In commit "doc: Improve guide for usage of assumeutxo" (6a5408dd7da4c31283cb4f632b3a7aa692a53ca3)
Maybe say "generate it yourself from another node" to be clear you need a different node to generate it.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1648975246)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
s/disc/disk/
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1648975246)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
s/disc/disk/
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649333656)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
For a followup, it might be convenient if height value defaulted to the last height value which could actually be loaded as a snapshot. It could also accept a "latest" string value in case the user does want to generate the latest snapshot.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649333656)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
For a followup, it might be convenient if height value defaulted to the last height value which could actually be loaded as a snapshot. It could also accept a "latest" string value in case the user does want to generate the latest snapshot.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649335464)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
It seems racy to unlock cs_main before calling CreateUTXOSnapshot. Not sure if there is a way to implement this in a race-free way without looping though. Could be something to follow up on later.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649335464)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
It seems racy to unlock cs_main before calling CreateUTXOSnapshot. Not sure if there is a way to implement this in a race-free way without looping though. Could be something to follow up on later.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649328350)
In commit "doc: Improve guide for usage of assumeutxo" (6a5408dd7da4c31283cb4f632b3a7aa692a53ca3)
Is it true that no pruning will take place between the snapshot and the tip? I haven't experimented but it looks like GetPruneRange will only return a minimum prune height for the snapshot chainstate, not the background chainstate.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649328350)
In commit "doc: Improve guide for usage of assumeutxo" (6a5408dd7da4c31283cb4f632b3a7aa692a53ca3)
Is it true that no pruning will take place between the snapshot and the tip? I haven't experimented but it looks like GetPruneRange will only return a minimum prune height for the snapshot chainstate, not the background chainstate.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649378681)
In commit "RPC: Add height parameter to dumptxoutset" (https://github.com/bitcoin/bitcoin/commit/2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
Documentation here is really terse. For a folluwup , I think it would be nice to add some of the documentation from the usage guide to the RPC documentation so it is more accessible.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1649378681)
In commit "RPC: Add height parameter to dumptxoutset" (https://github.com/bitcoin/bitcoin/commit/2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
Documentation here is really terse. For a folluwup , I think it would be nice to add some of the documentation from the usage guide to the RPC documentation so it is more accessible.
💬 ryanofsky commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1648979413)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
> This actually isn't a fully sufficient check because we could be missing the undo data. I am addressing this here: #29668
Can you say what the problem is in a code comment so it is clear what current behavior is? Comment can removed when the problem is fixed later.
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1648979413)
In commit "RPC: Add height parameter to dumptxoutset" (2cdce8fd9c93099db7e86ff33a1da8fbf0aaf364)
> This actually isn't a fully sufficient check because we could be missing the undo data. I am addressing this here: #29668
Can you say what the problem is in a code comment so it is clear what current behavior is? Comment can removed when the problem is fixed later.
💬 furszy commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649417438)
yes, thx
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649417438)
yes, thx
💬 furszy commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649425300)
yeah, good eye. `send()` and `walletprocesspsbt()` will also fail. Will add coverage for one of them.
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649425300)
yeah, good eye. `send()` and `walletprocesspsbt()` will also fail. Will add coverage for one of them.
💬 furszy commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649434850)
> I assume the `MAX_STANDARD_TX_WEIGHT` will be replaced by `max_tx_weight` (if present) during or after [29523](https://github.com/bitcoin/bitcoin/pull/29523) PR is merged?
Yes.
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649434850)
> I assume the `MAX_STANDARD_TX_WEIGHT` will be replaced by `max_tx_weight` (if present) during or after [29523](https://github.com/bitcoin/bitcoin/pull/29523) PR is merged?
Yes.
🤔 furszy reviewed a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2133442120)
> * To my understanding, the class `LegacyDataSPKM` is supposed to never write to the database (as `BerkeleyRODatabase` obviously doesn't support that), but `LegacyDataSPKM::CheckDecryptionKey` contains a code-path for that (for a necessary rewrite):
> https://github.com/achow101/bitcoin/blob/a8f8a2d5d0e2328cbe2e569c7e12f653d18f456d/src/wallet/scriptpubkeyman.cpp#L254-L255
> Should `CheckDecryptionKey` stay in `LegacyScriptPubKeyMan`?
It isn't really an issue because `CheckDecryptionKey
...
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2133442120)
> * To my understanding, the class `LegacyDataSPKM` is supposed to never write to the database (as `BerkeleyRODatabase` obviously doesn't support that), but `LegacyDataSPKM::CheckDecryptionKey` contains a code-path for that (for a necessary rewrite):
> https://github.com/achow101/bitcoin/blob/a8f8a2d5d0e2328cbe2e569c7e12f653d18f456d/src/wallet/scriptpubkeyman.cpp#L254-L255
> Should `CheckDecryptionKey` stay in `LegacyScriptPubKeyMan`?
It isn't really an issue because `CheckDecryptionKey
...
👍 tdb3 approved a pull request: "wallet: notify when preset + automatic inputs exceed max weight"
(https://github.com/bitcoin/bitcoin/pull/30309#pullrequestreview-2133551145)
re ACK for 72b226882fe2348a9a66aee1d8d21b4e2d275e68
Re-ran tests locally (exercising induced failures and received failures as expected).
`test_weight_limits()` implementations are almost identical between `wallet_fundrawtransaction` and `wallet_spend`. While this feels a bit redundant, these are separate functional tests so an alternative (like combining) wouldn't really make sense to do.
(https://github.com/bitcoin/bitcoin/pull/30309#pullrequestreview-2133551145)
re ACK for 72b226882fe2348a9a66aee1d8d21b4e2d275e68
Re-ran tests locally (exercising induced failures and received failures as expected).
`test_weight_limits()` implementations are almost identical between `wallet_fundrawtransaction` and `wallet_spend`. While this feels a bit redundant, these are separate functional tests so an alternative (like combining) wouldn't really make sense to do.
💬 tdb3 commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649519927)
Saw that the same expected failure occurs here when commenting out `return util::Error...` for 72b226882fe2348a9a66aee1d8d21b4e2d275e68.
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649519927)
Saw that the same expected failure occurs here when commenting out `return util::Error...` for 72b226882fe2348a9a66aee1d8d21b4e2d275e68.
💬 tdb3 commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649519554)
Re-ran this test (comment out `return util::Error...`) for 72b226882fe2348a9a66aee1d8d21b4e2d275e68. Failed as expected.
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1649519554)
Re-ran this test (comment out `return util::Error...`) for 72b226882fe2348a9a66aee1d8d21b4e2d275e68. Failed as expected.
💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1649531050)
I made the change to reflect different path string depending on OS. Let me know if this is what you meant. I was also debating doing an alternative way where I have 2 example types, one MacOS and other Unix, so that web docs like https://developer.bitcoin.org/reference/rpc/loadwallet.html show both examples.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1649531050)
I made the change to reflect different path string depending on OS. Let me know if this is what you meant. I was also debating doing an alternative way where I have 2 example types, one MacOS and other Unix, so that web docs like https://developer.bitcoin.org/reference/rpc/loadwallet.html show both examples.
📝 romanz opened a pull request: "rest: don't copy block data when sending binary response"
(https://github.com/bitcoin/bitcoin/pull/30321)
Also, change `HTTPRequest::WriteReply` to accept `std::string_view`.
(https://github.com/bitcoin/bitcoin/pull/30321)
Also, change `HTTPRequest::WriteReply` to accept `std::string_view`.
💬 maflcko commented on pull request "rest: don't copy block data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2183964434)
lgtm ACK 07770609afddaa1c730b431c207015f34b3a5f5f
A benchmark would be nice (for fun), but this looks good either way.
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2183964434)
lgtm ACK 07770609afddaa1c730b431c207015f34b3a5f5f
A benchmark would be nice (for fun), but this looks good either way.
💬 laanwj commented on pull request "rest: don't copy block data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2184047556)
LGTM ACK 07770609afddaa1c730b431c207015f34b3a5f5f
Nice, simple optimization.
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2184047556)
LGTM ACK 07770609afddaa1c730b431c207015f34b3a5f5f
Nice, simple optimization.