💬 ismaelsadeeq commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1838729200)
In a8bf940ae20ed5a03a1cc328212fcac3c3462b43
A comment explaining why you selected these magic numbers would be helpful.
similar to `tx_noinputs_size` or an indication if they are just random.

but in initializing CSP, and some defaults in `MakeCoin`

💬 ismaelsadeeq commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1838770918)
Since we are checking for Value equivalence, this can just be
`CheckValueEquivalence` for clarity

💬 ismaelsadeeq commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1838771982)
```suggestion
auto ret = std::mismatch(a_amts.begin(), a_amts.end(), b_amts.begin());
```

💬 ismaelsadeeq commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1838790179)
Ahh Makes sense

💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2471614064)
> Did you try to follow the approach suggested here: [#31088 (comment)](https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2413394142)? If not, can you say why it didn't work?

I would have to look deeper on how to do that.
But I don't see why it would be better than this approach. Deprecating here is easy and there will not be the need to modify code other than deleting the actual function.

🤔 ismaelsadeeq reviewed a pull request: "doc: mention `descriptorprocesspsbt` in psbt.md"
(https://github.com/bitcoin/bitcoin/pull/31277#pullrequestreview-2430761369)
Nice docs
ACK ebb6cd82baf8406454b18afcb8ccb4e1dde0d43e

🤔 ismaelsadeeq reviewed a pull request: "wallet, rpc: Settxfeerate"
(https://github.com/bitcoin/bitcoin/pull/31278#pullrequestreview-2430770541)
Nice fixing the ambiguity.

But generally, I’m rate Concept ~0 on setting static feerate for wallet transaction as a whole because of the current dynamic fee environment, it’s just a footgun.
It will make a transaction either pay too high or too low.

Feerate estimate from fee estimation algorithm should be used in most cases, and when that algorithm is deem inaccurate by users should they should pass the fee rate for each individual transaction.

We should lean towards deprecating this R
...

💬 ismaelsadeeq commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1838822782)
Its weird that you change this line to s/vb in previous commit and then revert it back to `s/kvB` here.

💬 ismaelsadeeq commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1838819668)
Should probably add a test for the utility of this, not just edge cases; i.e., set a fee rate, create a transaction, and verify that the fee rate matches.

💬 ismaelsadeeq commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1838824308)
fix-ups should be squashed according to `contributing.md` guidelines.

💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1838902865)
That was a mistake in the previous commit, as it's the ``settxfee`` function and not ``settxfeerate``.

💬 kevkevinpal commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#issuecomment-2471793963)
rebased to [6590b26](https://github.com/bitcoin/bitcoin/pull/29500/commits/6590b267015ad094dfcbacad521c30207fe199bd)

and I can address your latest comments soon

💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2471842227)
> Feerate estimate from fee estimation algorithm should be used in most cases, and when that algorithm is deem inaccurate by users should they should pass the fee rate for each individual transaction.

I agree that the algorithm should be used in most cases, and actually that's how it's working according to the RPC call definition. By default it uses the dynamic fee so this is only an extra option to set a static fee in case anyone needs too.
https://github.com/bitcoin/bitcoin/blob/b96cfae733
...

💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1839061111)
Good point, added!

💬 fjahr commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1839068782)
That's what we have the `-deprecatedrpc` parameter for

💬 fjahr commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1839069865)
This file didn't exist before 2024, so it should just say 2024

💬 ryanofsky commented on pull request "WIP: scripted-diff: Type-safe settings retrieval":
(https://github.com/bitcoin/bitcoin/pull/31260#issuecomment-2472025885)
Updated 416860fc360b3d5aa1a0782ab8f8454b46e6d657 -> 47e30b40eca0eabf85d45e91fc247a74f3a346e8 ([`pr/scripty.1`](https://github.com/ryanofsky/bitcoin/commits/pr/scripty.1) -> [`pr/scripty.2`](https://github.com/ryanofsky/bitcoin/commits/pr/scripty.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/scripty.1..pr/scripty.2)) getting the remaining binaries (not just test_bitcoin and bitcoind) to build, simplifying the way optional and default values are used, making many other cleanups and
...

💬 fjahr commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1839083891)
Looking back it seems like we haven't hidden them, just deprecated. That's also what I would suggest here.

💬 adamandrews1 commented on pull request "addrman: cap the `max_pct` to not exceed the maximum number of addresses":
(https://github.com/bitcoin/bitcoin/pull/31235#issuecomment-2472050919)
Code Review ACK https://github.com/bitcoin/bitcoin/commit/9c5775c331e02dab06c78ecbb58488542d16dda7

💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1839107296)
Do you have an example of deprecated RPC so I can compare on how it was done before?