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

💬 achow101 commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-2195406588)
ACK 73f0a6cbd0b628675028fbd5a37eff8115e7ccfe

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657625195)
```diff
diff --git a/src/random.h b/src/random.h
index e67e295b5f..f447216776 100644
--- a/src/random.h
+++ b/src/random.h
@@ -64,9 +64,11 @@
* (up to) the first 32 bytes of H are produced as output, while the last 32 bytes
* become the new RNG state.
*
- * During tests, the RNG is put into a special deterministic mode, in which the output of
- * all RNG functions, with the exception of GetStrongRandBytes(), is replaced with the
- * output of a deterministic RNG that is initiali
...

💬 maflcko commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1657628378)
https://cirrus-ci.com/task/5319226913718272?logs=ci#L1930

```
test 2024-06-26T19:28:29.157000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 157, in try_rpc
fun(*args, **kwds)
File "/ci_container_ba
...

👍 itornaza approved a pull request: "Mining interface followups, reduce cs_main locking, test rpc bug fix"
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2146255026)
Tested ACK a74b0f93efa1d9eaf5abc2f6591c44a632aec6ed

Code reviewed and validated all the initial refactor and appended fix changes as they were discovered during the course of this PR.

Cleaned up and rebuilt this commit running the unit, functional and extended tests that all pass.