💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1657544231)
Fixed.
(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.
(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
...
(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.
(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.
(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?
(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.
(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?
(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.
(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?
(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:
...
(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:
(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?
(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
...
(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\\\"")
...
```
(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)
(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.
(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
(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
...
(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
...
(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
...