💬 spikeyrock commented on issue "avoid using invalidateblock to directly test reorg behavior":
(https://github.com/bitcoin/bitcoin/issues/32531#issuecomment-2906624037)
maybe spin up two regtest nodes, mine a fork on one, and then connect them at just the right time to trigger the reorg. That way, the reorg happens "naturally" through P2P sync rather than forcing it.

🤔 rkrux reviewed a pull request: "rpc: Note in fundrawtransaction doc, fee rate is for package"
(https://github.com/bitcoin/bitcoin/pull/32607#pullrequestreview-2866228840)
ACK fe0432b1c4a10b74844c2dedefccfe340c0d3b10

This is a good note, helpful. I verified in the code that the outputs selected in the transaction have a corresponding `ancestor_bump_fees` param that is used in calculating the `TotalBumpFees` inside `CreateTransactionInternal` function.

https://github.com/bitcoin/bitcoin/blob/638a4c0bd8b53766faeb437244b2aae4eed28dcf/src/wallet/coinselection.h#L75-L76

https://github.com/bitcoin/bitcoin/blob/638a4c0bd8b53766faeb437244b2aae4eed28dcf/src/wallet
...

💬 rkrux commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#discussion_r2105759450)
I see it needs a full stop after `watch-only`.

💬 rkrux commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#discussion_r2105759505)
Nit:

```diff
- not the resulting transaction.",
+ not only the resulting transaction.",
```

💬 zaidmstrr commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#discussion_r2105780723)
This new PR https://github.com/bitcoin/bitcoin/pull/32438/commits/fa62a013a558338dc6ee5fb4cfd6fc7c782c301b removes this `flush()` from the codebase. I think it should also be removed from `chain.capnp` otherwise it will give build errors.

🤔 naiyoma reviewed a pull request: "rpc: generatetomany"
(https://github.com/bitcoin/bitcoin/pull/32468#pullrequestreview-2866255285)
I'm a bit confused about the current approach. Are you planning to add `generatetomany` and also extend `generateblock` to accommodate an array of addresses?

I suggest that you put one approach in your fork and indicate this clearly in your description—separation of concerns makes it easier to follow and review.

For example:
Approach 1
Approach 2 → Link to the PR in your fork

💬 polespinasa commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2906764458)
> I'm a bit confused about the current approach. Are you planning to add `generatetomany` and also extend `generateblock` to accommodate an array of addresses?

`generatetomany` will not be added. I have to delete that code.

💬 maflcko commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#issuecomment-2906765296)
> `generatetomany` will not be added. I have to delete that code.

Generally, it is best to just push the final version of the code, so that it is ready for further review. Prior versions of the code can trivially be retrieved via the commit hash (or a named alias), if needed.

💬 naiyoma commented on pull request "rpc: generatetomany":
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2105783773)
11 shouldn't be str
```diff
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -351,7 +351,7 @@ static RPCHelpMan generatetomany()
}},
RPCExamples{
"\nGenerate 11 blocks to two different addresses:\n"
- + HelpExampleCli("generatetomany", "\"11 '[\\\"bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v\\\", \\\"bcrt1qvr3qgyhw6y0e0zj97v0j5yc40xtpea4wqj0g43\\\"]'\"")
+ + HelpExampleCli("generatetomany", "11 '[\"bcrt1qal6p633hvwz2yp5mav0
...

💬 maflcko commented on issue "test: `feature_fee_estimation` failure after duplicate coinbase tx weight reservation fix":
(https://github.com/bitcoin/bitcoin/issues/32461#issuecomment-2906769514)
https://cirrus-ci.com/task/5716739608018944?logs=ci#L3079

💬 maflcko commented on pull request "rpc: Note in fundrawtransaction doc, fee rate is for package":
(https://github.com/bitcoin/bitcoin/pull/32607#issuecomment-2906770023)
> Interesting that the `feature_fee_estimation.py ` test fails in the CI coincidentally but seems unrelated to me as it passed in my system. Maybe a rerun/push would fix it?

See https://github.com/bitcoin/bitcoin/issues/32461

💬 maflcko commented on pull request "rpc, doc: clarify wallet version in getwalletinfo help":
(https://github.com/bitcoin/bitcoin/pull/32603#discussion_r2105786933)
I don't think this is right. The current wallet version is `169900` (for a freshly created descriptor wallet). However, I don't think a Bitcoin Core 16.99 client can open descriptor wallets at all.

Generally, all modules in Bitcoin Core moved away from the client version to use a module-specific and module-internal version. (You can see this when you look at all other files written (fee estimates, mempool, addr ...). Also the p2p version, but that again has been replaced by feature-messages.)
...

💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#discussion_r2105795801)
thx, done because it is less code and the suggestion was fine

💬 maflcko commented on pull request "rpc: Round verificationprogress to 1 for a recent tip":
(https://github.com/bitcoin/bitcoin/pull/32528#issuecomment-2906792097)
> Might warrant a release note: "Verification progress will be 100% if all known blocks are verified and timestamped within the last two hours when previously a stale tip might result in progress < 100%".

I consider the changes here only stylistic (similar to changing the `bitcoin-cli -color=...` colours). No one should be relying on the informational-only value anyway.

💬 maflcko commented on pull request "validation: Do less work in NeedsRedownload":
(https://github.com/bitcoin/bitcoin/pull/31714#issuecomment-2906794331)
Yeah, if we are fine with the rare edge case, it should also be fine to fully remove it, and instead replace the "unsupported utxo db" error message from reindex-chainstate to reindex. Alternatively, it could automatically force a reindex to avoid the user picking the wrong setting.

💬 rkrux commented on pull request "rpc, doc: clarify wallet version in getwalletinfo help":
(https://github.com/bitcoin/bitcoin/pull/32603#discussion_r2105799056)
Hmm I see. Thanks for highlighting. Descriptor wallets were indeed introduced after v16.99.

I'm not sure then what does the "client version" here refers to. Maybe it is referring to a wallet client somehow as mentioned above that versioning has moved to being module specific.

I will dig more to see how this can be made clearer.

👍 TheCharlatan approved a pull request: "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`"
(https://github.com/bitcoin/bitcoin/pull/29954#pullrequestreview-2866286522)
ACK d165ac8779b2b692007a7474c19cee4194946e75

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2105809870)
Done, thanks for the spot!

💬 yancyribbens commented on issue "Support `Accept` HTTP header in REST API":
(https://github.com/bitcoin/bitcoin/issues/32583#issuecomment-2906823581)
I'm not sure what is meant by two conflicting types really.

I was thinking it would be possible to make a request using either json or binary in the request HTTP header. For example:

```
curl -X POST https://bitcoin
-H 'Accept: application/json'
```

OR

```
curl -X POST https://bitcoin
-H 'Accept: application/octet-stream'
```

If no Accept content type is set then default to what it is now, json.

I'm not sure, is that what the code snippet above is doing? I don't really grok what's
...

💬 yancyribbens commented on pull request "rest: fetch spent transaction outputs by blockhash":
(https://github.com/bitcoin/bitcoin/pull/32540#issuecomment-2906824816)
> would it be OK to implement it in a separate PR?


I was thinking it might be not ideal to have this endpoint in addition to adding a content-type. Unless the plan is to deprecate this after content type is added? Maybe that's what is meant by the two conflicting types mentioned https://github.com/bitcoin/bitcoin/issues/32583#issuecomment-2901076206