🤔 glozow reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1848564888)
ACK bc9283c4415a932ec1eeb70ca2aa4399c80437b3

Did some light code review of the `P2PConnection` functions, mutation testing of `EncryptedP2PState`, and manually changing other functional tests to use v2 connections.

💬 russeree commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914628448)
> and are thus provably unspendable about 50% of the time as the data doesn't match a valid secp point.

Sorry for not understanding this, but why 50%? I thought compressed pubkeys covered almost everything on the X axis because the y is generated?

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

💬 theDavidCoen commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1914655681)
> @theDavidCoen "Default off everything" is not at all how Bitcoin Core development works. If it did, the node would by default not communicate with the network (`-networkactive`), broadcast wallet transactions (`-walletbroadcast`), persist the mempool between restarts (`-persistmempool`) etc. It would also mean you can just rename an option (e.g. `-permitbaremultisig` to `-disablebaremultisig`) to force the opposite default.
>
> How it actually (usually) works is the default parameters are c
...

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