💬 NicolasDorier commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1915766303)
@1440000bytes the repartition you are showing is on outputs count, not size. Since ordinal, utxoset size been from 5GB to 10GB. Even if they are margin in term of output count, in term of size, it takes around 50% of the UTXO Set at the moment.

I haven't looked how they put JPEG and JSON files on the chain exactly, I assumed it was with invalid P2PK, but didn't look closely into it.

💬 achow101 commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1915778170)
> I haven't looked how they put JPEG and JSON files on the chain exactly, I assumed it was with invalid P2PK, but didn't look closely into it.

Ordinal inscriptions don't use P2PK or bare multisig, they use P2TR with tapscripts. In order for an inscription to be in the blockchain, it must create and then spend a UTXO, so inscriptions themselves have no impact on the UTXO set. Ordinals may have an impact as there are people creating small outputs for their "rare" sats, but those are all perfect
...

💬 stratospher commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#issuecomment-1916028804)
ACK d82eafb173d6bfa98a59e86a845013cc8528b65d.

💬 stratospher commented on pull request "net: enable v2transport by default":
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1916079402)
ACK 0bef104.

💬 t-bast commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1916166471)
Another approach that may be simpler is to imbue with v3 semantics only once one of the anchor output is being spent in the mempool (because if none are spent, there's no basically nothing to do). This way we can pattern match on the anchor output script which is very specific:

```
OP_PUSHDATA(<some_public_key>) OP_CHECKSIG OP_IFDUP OP_NOTIF OP_16 OP_CHECKSEQUENCEVERIFY OP_ENDIF
```

I think that this approach shouldn't generate any false positive. Once such an anchor transaction is ident
...

💬 maflcko commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1470738750)
nit: Please don't encode the default value in the syntax of the source code. Otherwise, it is hard to change the default value. Also, it is confusing to have to change two places to change one value. You could just use `self.Arg<bool>(2)`.

💬 naumenkogs commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#issuecomment-1916362642)
Concept ACK

💬 josibake commented on pull request "net: enable v2transport by default":
(https://github.com/bitcoin/bitcoin/pull/29347#issuecomment-1916364458)
reACK https://github.com/bitcoin/bitcoin/pull/29347/commits/0bef1042ce6c459acb1de965cbccd98867a417f1

💬 naumenkogs commented on pull request "validation: fix misleading checkblockindex comments":
(https://github.com/bitcoin/bitcoin/pull/29299#issuecomment-1916369770)
ACK 9819db4ccaa03519a78d4d9ecce9f89f5be669e5

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

💬 petertodd commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1916377490)
@sdaftuar

> So I assume that means you don't believe that dropping the CPFP carveout rule should be a big deal either, is that a fair statement?

No, that's not a fair statement. While I have my reservations about CPFP, the fact is we've shipped a lot of Lightining implementations that rely on the CPFP carveout for safety and we have to continue to provide that functionality for the forseeable future. Indeed, this complexity is one reason why I'm dubious about rushing into the half-baked V
...

💬 naumenkogs commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1916379223)
Concept ACK

🤔 naumenkogs reviewed a pull request: "net: enable v2transport by default"
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1850607392)
ACK 0bef1042ce6c459acb1de965cbccd98867a417f1

💬 naumenkogs commented on pull request "net: enable v2transport by default":
(https://github.com/bitcoin/bitcoin/pull/29347#discussion_r1470830112)
i'm not sure we want to keep `default_to_v2`. It's kinda unused now, and I'm not sure future tests would ever find it useful.

👍 theStack approved a pull request: "net: enable v2transport by default"
(https://github.com/bitcoin/bitcoin/pull/29347#pullrequestreview-1850619543)
ACK 0bef1042ce6c459acb1de965cbccd98867a417f1

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1916411990)
> I dont really understand how this can affect coinselection, can you expand please? Thanks

Sorry, I was completely overthinking it! I was incorrectly thinking that we took `max_tx_fee` and `tx_fee_rate` as inputs to coin selection, but what we do instead:

1. Check the if the user provided a `fee_rate` (this fee rate would have already been sanity checked against `max_fee_rate`)
2. If not, get a fee rate by doing some checks against min fees and fee estimation
3. Check `max_tx_fee` *afte
...

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1470879755)
What is the point of parsing as signed integer and then casting to unsigned? Why not use `ParseUInt32` directly?

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1916447568)
@ismaelsadeeq thanks for walking through the scenario! Reading over what you posted, if `maxtxfee` is left as the default, then the user can set whatever they want for `maxfeerate` without any issues. Depending on the types of transactions they make, they are essentially setting a new, lower `maxtxfee`.

If the user is setting both, they could create weird scenarios where the `maxtxfee` is too low compared to the `maxtxfeerate` and vice versa. But thinking it through and with your examples, I
...

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1470885445)
perhaps introduce a new error type:

```suggestion
return util::Error{TransactionErrorString(TransactionError::MAX_FEE_RATE_EXCEEDED)};
```

💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1470882974)
nit: Clang-format while touching this line of code?


```cpp
static const uint32_t CURRENT_VERSION{2};