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

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914963270)
> it sounds like we will also always be using a public key with the SV2 key,

Yes, but we just derive the public key from the private key. There's no need to store it and check it. If it got corrupted, you just make a new one.

> what you are doing now is also incorrect and confusing in that you are serializing and deserializing 32 bytes with no indication it is meant for an `XOnlyPublicKey`

We don't make that distinction for Taproot wallet keys either. Any compressed key can be added to
...

📝 Sjors opened a pull request: "Stratum v2 Noise Protocol"
(https://github.com/bitcoin/bitcoin/pull/29346)
Parent PR #28983

This is the first part of the Stratum v2 Template Provider change that can be a standalone pull request.

The Noise Protocol Framework is defined here: https://noiseprotocol.org/noise.html

It's quite similar to BIP324. The main differences are explained here, including why Stratum v2 can't use BIP234 (yet): https://delvingbitcoin.org/t/stratum-v2-noise-protocol-bip324-nuggets/413

The implementation is based on revision 38, 2018-07-11 (most recent at the time of writin
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1915048388)
I opened a (draft) pull request for the Noise part of this: #29346