🚀 fanquake merged a pull request: "depends: patch libool out of libnatpmp/miniupnpc"
(https://github.com/bitcoin/bitcoin/pull/29298)

💬 ismaelsadeeq commented on pull request "doc: update `BroadcastTransaction` comment":
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1469501085)
Yes I agree with your point.
Reverting back with your initial suggestion.

Thanks @stickies-v

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914568318)
Why a convert a key to a plain text hex string if you can just store the bytes? `CKey` also takes care to not leave key material dangling in memory in temporary variables.

💬 maflcko commented on issue "Discussion: Upgrading to C++20":
(https://github.com/bitcoin/bitcoin/issues/23363#issuecomment-1914568330)
Not sure if there is any value in `std::format`, because it is locale dependent. Seems better to stick to tinyformat?

💬 NicolasDorier commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914570114)
Concept ACK.

However, note that from the time this PR run on a node, its UTXO Set hash will start to differ from any peer that didn't start using this PR at the same time.

This can be solved by cleaning up the UTXO Set when the node starts. Unsure how expensive would it be.

💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914583600)
My question was why not handle StratumV2 keys the same way we are handling Tor and I2P identity keys and not use `CKey` at all. This avoids needing to add generic `Serialize/Deserialize` methods to `CKey` just for this specific use case, and also avoids needing to go write a specific SV2 key serialization format and corresponding `DecodeSV2Secret` method for `CKey`.

💬 ismaelsadeeq commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1469517392)
I dont understand why you said it is not a wallet option.
The `OptionsCategory` of this startup option is `WALLET`.
And its only used in the wallet.

Can you please expand on your comment.
Thanks

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