💬 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.

💬 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.

💬 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

💬 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.

💬 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.

🤔 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
...

👍 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.

💬 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.

💬 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.

💬 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.

📝 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`.

💬 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.

💬 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.

💬 laanwj commented on pull request "rest: don't copy block data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#discussion_r1649705033)
Not sure if a `Span<byte>` or string_view is best suited here. In principle we're dealing with binary data here, not necessarily a string.

📝 ismaelsadeeq opened a pull request: "[test]: prevent `create_self_transfer` failure when target weight is below tx weight"
(https://github.com/bitcoin/bitcoin/pull/30322)
This is a simple test PR that does two things:


1. Prevents `create_self_transfer` failure when the `target_weight` is too low, i.e., below the tx weight.
2. Addresses some review comments from https://github.com/bitcoin/bitcoin/pull/30162, which are:
- Use `WITNESS_SCALE_FACTOR` instead of the magic number 4.
- Use `PADDING_OFFSET` instead of the magic number 3.
- Raise an exception if the output value is less than or equal to zero in `create_self_transfer`.
This prevents cre
...

💬 ismaelsadeeq commented on pull request "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)":
(https://github.com/bitcoin/bitcoin/pull/30162#issuecomment-2184052503)
> > Would be nice to have this work for create_self_transfer_multi as well
>
> I will pick this up and the outstanding review comments.

Opened #30322 that addresses most comments here and also prevent `create_self_transfer` failure when target weight is below tx weight, you can test this by passing a low `target_weight` to `create_self_transfer`.

🤔 fjahr reviewed a pull request: "assumeutxo: Don't load a snapshot if it's not in the best header chain"
(https://github.com/bitcoin/bitcoin/pull/30320#pullrequestreview-2133844775)
Concept ACK

💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1649733840)
nit: Not sure if users understand what "base block" is. Maybe make it "snapshot base block".

💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1649739144)
done

💬 fjahr commented on pull request "prune, rpc: Check undo data when finding pruneheight":
(https://github.com/bitcoin/bitcoin/pull/29668#discussion_r1649739149)
Not sure why this is still needed when we return above if `!first_unpruned.pprev` but I've added it since it won't hurt.