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