🤔 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

💬 sipa commented on issue "Support `Accept` HTTP header in REST API":
(https://github.com/bitcoin/bitcoin/issues/32583#issuecomment-2906828308)
I don't understand the point of this. The format is already specified using the extension, which seems more convenient to me than needing an additional header to specify the content type, and having two distinct ways of specifying it seems significantly worse than just having one.

> I'm not sure what is meant by two conflicting types really.

I suspect Marco means: what do you do if someone requests `/block/$BLOCKHASH.bin`, but with `Accept: application/json` header.

💬 TheBlueMatt commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-2906834205)
The get transaction RT is the long pole mostly because Bitcoin Core won't respond to the request until it finishes validating the block (but will send the initial compact block prior to doing so). This makes it pretty slow depending on your peer, but also means that requesting it from many peers may be advantageous (because some peers will validate the block faster). Responding to the message should be ~free for the peers.

It might also be worth pointing out that dropping unsolicited messages p
...

💬 yancyribbens commented on issue "Support `Accept` HTTP header in REST API":
(https://github.com/bitcoin/bitcoin/issues/32583#issuecomment-2906837470)
> The format is already specified using the extension, which seems more convenient to me than needing an additional header to specify the content type,

Oh, if one can say `.bin` or `.json` in the request to specify the expected content format that's great.

In the PR https://github.com/bitcoin/bitcoin/pull/32540

> We can significantly optimize it by adding a new REST endpoint, using a binary response format (returning a collection of spent txout lists, one per each block transaction)

So I wa
...

💬 sipa commented on issue "Support `Accept` HTTP header in REST API":
(https://github.com/bitcoin/bitcoin/issues/32583#issuecomment-2906838614)
I suggest you read the [documentation](https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md) on the REST interface before commenting on it.

#32540 adds a new REST endpoint, which provides information that no current endpoint provides. Like all REST endpoints, it provides this information in a number of formats, based on the extension of the filename used in the request.

💬 yancyribbens commented on issue "Support `Accept` HTTP header in REST API":
(https://github.com/bitcoin/bitcoin/issues/32583#issuecomment-2906839560)
> I suggest you read the [documentation](https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md) on the REST interface before commenting on it.

> https://github.com/bitcoin/bitcoin/pull/32540 adds a new REST endpoint, which provides information that no current endpoint provides. Like all REST endpoints, it provides this information in a number of formats, based on the extension of the filename used in the request.

I see that now. My mistake.