📝 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 &
...

💬 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

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

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

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

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

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

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

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

💬 sipa commented on pull request "Several randomness improvements":
(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.

💬 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) {
```

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

💬 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

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

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

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

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

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

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