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

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

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657560540)
nit in c8da96061a777a4206e05f7cde0f5de6f3ce3e8e: Are you sure about "never reseeded"?

It should be possible to call `SeedRandomForTest` twice, to toggle the seed, or reseed with the existing seed, no?

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

I don't think so?

```diff
diff --git a/src/random.h b/src/random.h
index ab4e049817..fd05e7e575 100644
--- a/src/random.h
+++ b/src/random.h
@@ -28,8 +28,8 @@
* The following (classes of) functions interact with that state by mixing in new
* entropy, and optionally extracting random output from it:
*
- * - The GetRand*() class of functions, as well as construction of FastRandomContext objects,
- * perform 'fast' seeding, consisting of mixing in:
...

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657561740)
Ah, I looked at the previous force push :sweat_smile:

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657563307)
Eh, I see how this is confusing.

It can obviously be reinitialized (resetting it), but it doesn't every gather real entropy, and is unaffected by `RandAddPeriod()` or `RandAddEvent()`.

Feel like proposing some language that would be clearer?

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2195320994)
re-ACK 1a2eae1f6d2487e8363b660e68c22474c317921e 📔

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 1a2eae1f6d2487e8363b
...

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1657568396)
```c++
...
+ "\nLoad wallet using absolute path (Unix):\n"
+ HelpExampleCli("loadwallet", "\"/home/myusername/specialWallet/\"")
+ HelpExampleRpc("loadwallet", "\"/home/myusername/specialWallet/\"")
+ "\nLoad wallet using absolute path (Windows):\n"
+ HelpExampleCli("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
+ HelpExampleRpc("loadwallet", "\"C:\\Users\\myusername\\specialWallet\\\"")
...
```

🚀 achow101 merged a pull request: "refactor, wallet: get serialized size of `CRecipient`s directly"
(https://github.com/bitcoin/bitcoin/pull/30050)

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657596728)
Maybe "never strengthened" or "never fed further bytes" instead of "never reseeded"? But just a nit. Anything is fine here.