💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632290445)
Hmm. The intent was that this is supported behavior; `GetStrongRandBytes()` never sets `allow_deterministic`, and thus in deterministic mode would still return from the normal fully-seeded RNG (because I don't want to introduce anything the code path of `GetStrongRandBytes` that could return anything but high quality randomness). This means that if `GetStrongRandBytes` is called anywhere in test code that enables deterministic randomness, the condition you state occurs.
It's probably possible
...
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632290445)
Hmm. The intent was that this is supported behavior; `GetStrongRandBytes()` never sets `allow_deterministic`, and thus in deterministic mode would still return from the normal fully-seeded RNG (because I don't want to introduce anything the code path of `GetStrongRandBytes` that could return anything but high quality randomness). This means that if `GetStrongRandBytes` is called anywhere in test code that enables deterministic randomness, the condition you state occurs.
It's probably possible
...
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632292752)
Yes, because `m_cache_entry_expiration` is expressed in microseconds anyway; no need to reduce accuracy. I've moved the behavior change to a separate commit.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632292752)
Yes, because `m_cache_entry_expiration` is expressed in microseconds anyway; no need to reduce accuracy. I've moved the behavior change to a separate commit.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632293002)
I think I was inspired by Python's `random.expovariate`, but I agree "exp" is much more naturally associated with "exponential". Done.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632293002)
I think I was inspired by Python's `random.expovariate`, but I agree "exp" is much more naturally associated with "exponential". Done.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632293085)
Done (I left the "instead" in place).
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632293085)
Done (I left the "instead" in place).
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632293173)
Nice catch. Done, and added a comment to explain.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632293173)
Nice catch. Done, and added a comment to explain.
💬 Gary19751957 commented on pull request "refactor: performance-for-range-copy in psbt.h":
(https://github.com/bitcoin/bitcoin/pull/30253#discussion_r1632294059)
```suggestion
for (const auto& sig_pair : partial_sigs) {
```
(https://github.com/bitcoin/bitcoin/pull/30253#discussion_r1632294059)
```suggestion
for (const auto& sig_pair : partial_sigs) {
```
💬 EthanHeilman commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632311931)
That makes sense. This improved organization is worth it.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632311931)
That makes sense. This improved organization is worth it.
💬 maflcko commented on pull request "ci: move ASan job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632312246)
Is this a bug outside of GitHub Actions? If yes, it would be good to have exact steps to test locally. Maybe even in a separate pull request.
<!-- I presume this can be removed once container engines have it enabled by default? https://www.github.com/containers/podman/issues/19761
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1632312246)
Is this a bug outside of GitHub Actions? If yes, it would be good to have exact steps to test locally. Maybe even in a separate pull request.
<!-- I presume this can be removed once container engines have it enabled by default? https://www.github.com/containers/podman/issues/19761
👍 tdb3 approved a pull request: "test: Remove redundant verack check"
(https://github.com/bitcoin/bitcoin/pull/30252#pullrequestreview-2106242208)
ACK fa4f453281e1b38ec4bc87eadbfcfee11d6ea530
Makes sense. Version handshake occurs before ping/pong.
As a sanity check, ran functional tests to see the tests continue to function properly (all passed). Also viewed node traffic between two regtest nodes, and saw ping/pong occur after version/verack exchange.
(https://github.com/bitcoin/bitcoin/pull/30252#pullrequestreview-2106242208)
ACK fa4f453281e1b38ec4bc87eadbfcfee11d6ea530
Makes sense. Version handshake occurs before ping/pong.
As a sanity check, ran functional tests to see the tests continue to function properly (all passed). Also viewed node traffic between two regtest nodes, and saw ping/pong occur after version/verack exchange.
💬 tdb3 commented on pull request "test: Remove redundant verack check":
(https://github.com/bitcoin/bitcoin/pull/30252#discussion_r1632312842)
nit:
`check it by waiting for a ping`
The code waits for successful pongs rather than pings.
(https://github.com/bitcoin/bitcoin/pull/30252#discussion_r1632312842)
nit:
`check it by waiting for a ping`
The code waits for successful pongs rather than pings.
💬 EthanHeilman commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632314639)
1. If I understand, you don't want to replace `MixExtract` entirely when the deterministic test RNG is used, because want `MixExtract` to still run and add/mix entropy in case someone decides to generate a keying material while the deterministic test RNG is on. This was what I was missing.
2. I like the name deterministic mode because it makes it more clear this is a RNG wide change.
3. Thought I had after waking up this morning was to change this to `bool fail_on_deterministic_mode`. Thus
...
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632314639)
1. If I understand, you don't want to replace `MixExtract` entirely when the deterministic test RNG is used, because want `MixExtract` to still run and add/mix entropy in case someone decides to generate a keying material while the deterministic test RNG is on. This was what I was missing.
2. I like the name deterministic mode because it makes it more clear this is a RNG wide change.
3. Thought I had after waking up this morning was to change this to `bool fail_on_deterministic_mode`. Thus
...
📝 maflcko opened a pull request: "log: use error level for critical log messages"
(https://github.com/bitcoin/bitcoin/pull/30255)
This picks up the first commit from https://github.com/bitcoin/bitcoin/pull/29231, but extends it to also cover cases that were missed in it.
As per https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging, LogError should be used for severe problems that require the node to shut down.
(https://github.com/bitcoin/bitcoin/pull/30255)
This picks up the first commit from https://github.com/bitcoin/bitcoin/pull/29231, but extends it to also cover cases that were missed in it.
As per https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging, LogError should be used for severe problems that require the node to shut down.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632321305)
How about having an `always_use_real_rng` bool instead, with meaning `!allow_deterministic`. That means the "deterministic" terminology would be more clearly referring to the mode (is the deterministic PRNG enabled), but not to the produced numbers.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632321305)
How about having an `always_use_real_rng` bool instead, with meaning `!allow_deterministic`. That means the "deterministic" terminology would be more clearly referring to the mode (is the deterministic PRNG enabled), but not to the produced numbers.
🤔 tdb3 reviewed a pull request: "refactor: performance-for-range-copy in psbt.h"
(https://github.com/bitcoin/bitcoin/pull/30253#pullrequestreview-2106277965)
Approach ACK.
Less copying makes sense. Briefly searched for other instances in psbt.h where potentially unnecessary copying is done.
Would it also make sense to adjust these references to const (or is there a specific reason why const should not be used here)? Adjusted to `const auto& entry` and ran unit and functional tests (no issues observed).
https://github.com/bitcoin/bitcoin/blob/fab01b5220c28a334b451ed9625bd3914c48e6af/src/psbt.h#L355-L359
https://github.com/bitcoin/bitcoin/blo
...
(https://github.com/bitcoin/bitcoin/pull/30253#pullrequestreview-2106277965)
Approach ACK.
Less copying makes sense. Briefly searched for other instances in psbt.h where potentially unnecessary copying is done.
Would it also make sense to adjust these references to const (or is there a specific reason why const should not be used here)? Adjusted to `const auto& entry` and ran unit and functional tests (no issues observed).
https://github.com/bitcoin/bitcoin/blob/fab01b5220c28a334b451ed9625bd3914c48e6af/src/psbt.h#L355-L359
https://github.com/bitcoin/bitcoin/blo
...
💬 maflcko commented on pull request "test: Remove redundant verack check":
(https://github.com/bitcoin/bitcoin/pull/30252#discussion_r1632322469)
Thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/30252#discussion_r1632322469)
Thanks, fixed.
👍 tdb3 approved a pull request: "test: Remove redundant verack check"
(https://github.com/bitcoin/bitcoin/pull/30252#pullrequestreview-2106279027)
ACK fa6627706580ea213fc5df23fe990f1feedec11d
(https://github.com/bitcoin/bitcoin/pull/30252#pullrequestreview-2106279027)
ACK fa6627706580ea213fc5df23fe990f1feedec11d
💬 maflcko commented on pull request "refactor: performance-for-range-copy in psbt.h":
(https://github.com/bitcoin/bitcoin/pull/30253#issuecomment-2156654277)
> Less copying makes sense. Briefly searched for other instances in psbt.h where potentially unnecessary copying is done.
The examples you provided are already const references. In C++, when calling a member method that is marked `const`, member access will be const. Moreover, `auto&` is a reference that hides the type, including the const-ness, so `const auto&` and `auto&` should be identical in this context (const references that do not copy), and no further change should be needed.
(https://github.com/bitcoin/bitcoin/pull/30253#issuecomment-2156654277)
> Less copying makes sense. Briefly searched for other instances in psbt.h where potentially unnecessary copying is done.
The examples you provided are already const references. In C++, when calling a member method that is marked `const`, member access will be const. Moreover, `auto&` is a reference that hides the type, including the const-ness, so `const auto&` and `auto&` should be identical in this context (const references that do not copy), and no further change should be needed.
👍 EthanHeilman approved a pull request: "Several randomness improvements"
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2106304644)
ACK 125509f395a214465ebf379164e41e4c6d19d443
I have not tested the code. All my comments have been addressed.
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2106304644)
ACK 125509f395a214465ebf379164e41e4c6d19d443
I have not tested the code. All my comments have been addressed.
💬 marcofleon commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#issuecomment-2156686118)
> My only note: I think it would be worthwhile to point out in the PR description what changes you have made to the original harness and why they were necessary.
Done. Thanks for all the help @dergoegge.
(https://github.com/bitcoin/bitcoin/pull/30230#issuecomment-2156686118)
> My only note: I think it would be worthwhile to point out in the PR description what changes you have made to the original harness and why they were necessary.
Done. Thanks for all the help @dergoegge.
💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-2156694207)
ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 🐿
<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: ACK 429ec1aaaaafab150f11e27fcf
...
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-2156694207)
ACK 429ec1aaaaafab150f11e27fcf132a99b57c4fc7 🐿
<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: ACK 429ec1aaaaafab150f11e27fcf
...