💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657487890)
Space

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1657504370)
I added a commit, that add more information about the modes.

On master b27afb7fb774809c9c075d56e44657e0b607a00c
This is the help output
```
2. estimate_mode (string, optional, default="conservative") The fee estimate mode.
Whether to return a more conservative estimate which also satisfies
a longer history. A conservative estimate potentially returns a
higher feerate and is more likely to be sufficient for the desired

...

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2195282495)
I've addressed the outstanding comments, I believe, including:
* Rename `allow_deterministic` to `always_use_real_rng` (with inverted meaning).
* Dropped `GetRand()` (but not `GetRand(nRange)`), inlining it instead.
* Removed `GetOSRand` from the public random.h interface, and moved it into the anonymous namespace.
* Fixed/improved a few comments
* Dropped most of the specializations in `randbits<BITS>`.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657537328)
Done.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657539130)
Moved `GetOSRand` to the anonymous namespace (meaning it can stay in its relative location in random.cpp, reducing the diff size), and dropped it from random.h.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657539323)
Done, in a new commit.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657539584)
Fixed.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2195289826)
Alright, should hopefully be ready to review.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657543406)
Yeah, I don't expect this to matter for `FastRandomContext`, where the production of randomness is expensive enough that it doesn't matter. But for `InsecureRandomContext`, I want to make sure the compiler creates a suitably specialized instance of `randbits` for use in `randbool` for example. While not done in this PR, the original reason for starting to work on this PR is because I want a fast `InsecureRandomContext` for use in #30286.

That said, you're right that all the different manual s
...

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657543513)
Fixed.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657543755)
Nice, done.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657544090)
Fixed, in the "cleanup" commit.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657544231)
Fixed.

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1657551280)
How about just hard-coding two examples- one for Linux and other for Windows? since I generally agree with
> I am sure a user can figure out what an absolute path means without having to see it in the docs.

But there was a request [here](https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1649343879) for showing the paths for different OS.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#issuecomment-2195301224)
> I'm not completely sure about [584e524](https://github.com/bitcoin/bitcoin/commit/584e524eb57444d7192df1049cafde9ccc480406). The commit description says
>
> > Originally these tests verified that at a SelectCoins level that a
> > solution with fewer inputs gets preferred at high feerates, and a
> > solution with more inputs gets preferred at low feerates. This outcome
> > relies on the behavior of BnB, so we move these tests under the umbrella
> > of BnB tests.
>
> It is true that th
...

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657552675)
I see, thanks for clarifying. I think I cleared out everything that I don’t use.

💬 murchandamus commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1657553731)
And then actually also removed it as a variable in the function by assuming that there generally is only one output.

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657554069)
Forgot to address this?

💬 achow101 commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#issuecomment-2195304200)
ACK a9c7300135f0188daa5bce5491e2daf2dd8da8ae

> Have to admit that I'm not fan of the multiple `CTxOut` creations and `GetScriptForDestination` calls when we could cache the `CTxOut` early in the process, leaving the silent payment destinations empty, just so they can be modified later on

I don't like that since it overloads the meaning of an empty scriptPubKey. This would also not work as well for future silent payments versions since information about the version.

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657554962)
nit in 326c76909fd7add514d320bc76a576af31d40dc0: Why move this?