👍 kristapsk approved a pull request: "doc: update `BroadcastTransaction` comment"
(https://github.com/bitcoin/bitcoin/pull/29308#pullrequestreview-1848637945)
ACK 31cce4a1bdbb48f57996615ee6c686e456cc0bea

👍 maflcko approved a pull request: "Nuke adjusted time from validation (attempt 2)"
(https://github.com/bitcoin/bitcoin/pull/28956#pullrequestreview-1848654967)
lgtm ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2 🤽

<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: lgtm ACK ff9039f6ea876bab
...

💬 dergoegge commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1914713579)
* [CURRENT_VERSION](https://github.com/bitcoin/bitcoin/blob/3a41d8ca8481ce7f85b1c9a9638d63e8e5fa5285/src/primitives/transaction.h#L299) should probably also change to `uint32_t`

* CI failes [here](https://github.com/bitcoin/bitcoin/blob/3a41d8ca8481ce7f85b1c9a9638d63e8e5fa5285/src/test/fuzz/util.cpp#L46-L48) due to UB, can be fixed by changing to `ConsumeIntegral<uint32_t>()`

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1469616586)
```suggestion
tx.version = newVersion;
```

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1469618152)
```suggestion
CMutableTransaction::CMutableTransaction() : version{CTransaction::CURRENT_VERSION}, nLockTime{0} {}
```

nit: `{}` cat be used to check at compile time that the integral type does not change the sign or is truncated

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1469618343)
same

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1469623110)
> [9f36e59](https://github.com/bitcoin/bitcoin/commit/9f36e591c551ec2e58a6496334541bfdae8fdfe5)
>
> i see that this is a copy-paste from another place, but since it's technically green (and i want to sure it still makes sense after your change), can you explain what is this about? especially `the returned desirable service flags are guaranteed to not change dependent on state`

Yeah sure. It means that when `NODE_NONE` is provided, the function ensures to consistently return the default ser
...

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1469628831)
Sure. As it would be really nice to progress on this PR, will leave it follow-up. Thanks.

💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1469626733)
sure. Will do if I have to re-touch.

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914768513)
The Tor and I2P keys are not generated by us. We get them via a JSON RPC and pass them back that way. We don't use them ourselves for decryption and encryption. The passing around as plain text is suboptimal, because we're not clearing the key data from memory.

With Stratum v2 we _do_ use the key. We need its pubkey, perform (Elligator-Swift) ECDH with it, etc. That's what a CKey is for.

💬 BrandonOdiwuor commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1469664573)
fixed

💬 BrandonOdiwuor commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1469665865)
moved the check up `run_checks(...)`

👋 BrandonOdiwuor's pull request is ready for review: "test: Handle functional test disk-full error"
(https://github.com/bitcoin/bitcoin/pull/29335)

💬 maflcko commented on pull request "fuzz: Print coverage summary after run_once":
(https://github.com/bitcoin/bitcoin/pull/29329#issuecomment-1914800796)
Apologies for the force pushes, but I forgot to sort the summary

💬 maflcko commented on pull request "fuzz: Print coverage summary after run_once":
(https://github.com/bitcoin/bitcoin/pull/29329#issuecomment-1914807374)
Example diff:


💬 maflcko commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1469688234)
What is the total size of the directory when no test dir is cleaned up and all tests are run?

👍 dergoegge approved a pull request: "fuzz: Print coverage summary after run_once"
(https://github.com/bitcoin/bitcoin/pull/29329#pullrequestreview-1848840989)
ACK fab97d81ce3740509dbbe9270ca67a1b65b00c72

💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914863218)
> With Stratum v2 we do use the key. We need its pubkey, perform (Elligator-Swift) ECDH with it, etc. That's what a CKey is for.

Your 1st objection to using `CPrivKey` was that `CPrivKey` is always meant to be used in conjunction with a public key (implying that this SV2 key is not), but it sounds like we will also always be using a public key with the SV2 key, so I don't understand your 1st objection to using `CPrivKey` or why we would need a "dummy public key."

Your second point makes se
...

📝 maflcko opened a pull request: "rpc: Do not wait for headers inside loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/29345)
While the `loadtxoutset` default 10 minute timeout is convenient when it is sufficient, it may cause hassle where it is not. For example:

* When P2P connections are missing, it seems better to abort early than wait for the timeout.
* When the 10 minute timeout is not sufficient, the RPC will have to be called again, so a check or loop is needed outside the RPC either way. So might as well remove the loop inside the RPC.

💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx in RPC results are off":
(https://github.com/bitcoin/bitcoin/issues/29328#issuecomment-1914922108)
The RPCs should probably be fixed to never return fake values.

It is pretty easy to tell when the `nTx` value is fake. You can just call [`IsValid()`](https://github.com/bitcoin/bitcoin/blob/478ac185be49feffd1231366c07e06068683f941/src/chain.h#L306), and if it returns true, the `nTx` value is real, and if it returns false, the value is fake.

It is less straightforward to determine when the `nChainTx` is fake, but we could introduce a helper function to do that which could be used by these
...