💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1632236142)
Done in https://github.com/bitcoin/bitcoin/pull/30252, also mentioning `fSuccessfullyConnected`. Feel free to NACK/ACK/nit.
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1632236142)
Done in https://github.com/bitcoin/bitcoin/pull/30252, also mentioning `fSuccessfullyConnected`. Feel free to NACK/ACK/nit.
📝 maflcko opened a pull request: "refactor: performance-for-range-copy in psbt.h"
(https://github.com/bitcoin/bitcoin/pull/30253)
A copy of the partial signatures is not required before serializing them.
For reference, this was found by switching the codebase to `std::span` (not sure why it wasn't found with `Span` though):
```
./psbt.h:240:23: error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
240 | for (auto sig_pair : partial_sigs) {
| ^
| const &
...
(https://github.com/bitcoin/bitcoin/pull/30253)
A copy of the partial signatures is not required before serializing them.
For reference, this was found by switching the codebase to `std::span` (not sure why it wasn't found with `Span` though):
```
./psbt.h:240:23: error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
240 | for (auto sig_pair : partial_sigs) {
| ^
| const &
...
💬 maflcko commented on pull request "refactor: performance-for-range-copy in psbt.h":
(https://github.com/bitcoin/bitcoin/pull/30253#issuecomment-2156448257)
Reference CI run: https://github.com/bitcoin/bitcoin/runs/25847037350
(https://github.com/bitcoin/bitcoin/pull/30253#issuecomment-2156448257)
Reference CI run: https://github.com/bitcoin/bitcoin/runs/25847037350
📝 theStack opened a pull request: "test: doc: fix units in tx-size standardness test (s/vbytes/weight units)"
(https://github.com/bitcoin/bitcoin/pull/30254)
This small fixup PR is a late follow-up for #17947 (commit 4537ba5f21ad8afb705325cd8e15dd43877eb28f), where the wrong units has been used in the comments for the large tx composition.
(https://github.com/bitcoin/bitcoin/pull/30254)
This small fixup PR is a late follow-up for #17947 (commit 4537ba5f21ad8afb705325cd8e15dd43877eb28f), where the wrong units has been used in the comments for the large tx composition.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632256491)
This is done in commit e5658e7a40f03b0b07b1f83f22a9d834bc8fd5c6 which only improves the code organization. Specifically here the reason for moving is to have all the non-exposed functions in `random.cpp` within one anonymous `namespace` at the start of the file, and all public one below.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632256491)
This is done in commit e5658e7a40f03b0b07b1f83f22a9d834bc8fd5c6 which only improves the code organization. Specifically here the reason for moving is to have all the non-exposed functions in `random.cpp` within one anonymous `namespace` at the start of the file, and all public one below.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632268413)
I see how this is confusing; `MixExtract` is indeed a deterministic function, and arguably, all of `RNGState` is deterministic when considered in isolation.
The "determinism" referred here is the introduction of a new deterministic mode for the entire RNG (module-wide, including seeding/initializations/hardware, ...). In order to do that, a separate RNG is embedded within `RNGState` which is only ever explicitly initialized, and if it is, `MixExtract` can be asked to draw its output from ther
...
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1632268413)
I see how this is confusing; `MixExtract` is indeed a deterministic function, and arguably, all of `RNGState` is deterministic when considered in isolation.
The "determinism" referred here is the introduction of a new deterministic mode for the entire RNG (module-wide, including seeding/initializations/hardware, ...). In order to do that, a separate RNG is embedded within `RNGState` which is only ever explicitly initialized, and if it is, `MixExtract` can be asked to draw its output from ther
...
👍 tdb3 approved a pull request: "test: doc: fix units in tx-size standardness test (s/vbytes/weight units)"
(https://github.com/bitcoin/bitcoin/pull/30254#pullrequestreview-2106188976)
cr ACK
Normally I would think this change could be included in a future change to `test_IsStandard`, but there is value in preventing confusion and not hoping that a future reviewer/author remembers to include this change.
(https://github.com/bitcoin/bitcoin/pull/30254#pullrequestreview-2106188976)
cr ACK
Normally I would think this change could be included in a future change to `test_IsStandard`, but there is value in preventing confusion and not hoping that a future reviewer/author remembers to include this change.
💬 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.