💬 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};

💬 ajtowns commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1916502753)
Isn't this check a little bit later than ideal? Wouldn't it make more sense to check both maxfeerate and maxburnamount as part of `signrawtransactionwith*` and `walletprocesspsbt` so that you never create a tx that can lose your money, rather than creating it and hoping that it won't find its way onto the network via some method other than rpc with default args?

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1916514936)
> If bitcoin core is not generating this key, then it seems reasonable to have the pubkey along with it so that it can be verified. If bitcoin core is generating this key, I don't see why we are writing it to disk unencrypted.

We generate the key for Stratum v2. We don't generate the key for Tor and I2P. We also don't encrypt those.

I don't see how encryption could work for a server application like a Template Provider. Putting the password in a config file adds complexity and no security
...

💬 kevkevinpal commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#issuecomment-1916529533)
Concept ACK [ebb842a](https://github.com/bitcoin/bitcoin/pull/29335/commits/ebb842ae4fbb20d2933e6852afd055bedeacf8c2)

💬 kevkevinpal commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1470966212)
I think we can reword this to
`Tests aborted: Insufficient space available in directory: {tmpdir}`

Since the device may have space but the `tmpdir` we are using might have insufficient space

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1470968822)
Interestingly, in https://github.com/bitcoin/bitcoin/blob/411ba32af21a56efa0a570b6aa8bf8f035410230/src/rpc/mempool.cpp#L89

`sendrawtransaction` takes a fee_rate argument, which is used to calculate a `max_tx_fee` based on the size of the transaction, and this `max_tx_fee` is passed to `BroadcastTransaction`. This means a user could typo an insane `maxfeerate` when calling `sendrawtransaction` that bypasses the default max tx fee.

Perhaps it does make more sense to have `BroadcastTransactio
...

💬 glozow commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#issuecomment-1916562018)
> Isn't this check a little bit later than ideal? Wouldn't it make more sense to check both maxfeerate and maxburnamount as part of `signrawtransactionwith*` and `walletprocesspsbt` so that you never create a tx that can lose your money

I think it's indeed good to have that, but isn't it still possible that transactions submitted via `submitpackage` was created somewhere else?