🤔 jonatack reviewed a pull request: "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses"
(https://github.com/bitcoin/bitcoin/pull/26859#pullrequestreview-1859765921)
Post-merge ACK b851c5385d0a0acec4493be1561cea285065d5dc

💬 Empact commented on pull request "refactor: Remove excess reserve() call for SecureString":
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1924287646)
@knst can you explain or cite a reference as to why this is no longer required? I would expect the implementation of `reserve` to be relative to the supplied allocator. Is it because size change is inexpensive with a reserved page via `LockedPoolManager`?

💬 maflcko commented on pull request "refactor: Remove nChainTx from consensus":
(https://github.com/bitcoin/bitcoin/pull/29349#discussion_r1476372560)
Fixed. (Implemented my suggestion)

💬 Empact commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#discussion_r1476384709)
Why not enumerate all combinations of services and connection types here?

🚀 achow101 merged a pull request: "refactor: Fix timedata includes"
(https://github.com/bitcoin/bitcoin/pull/29361)

💬 Empact commented on pull request "test: Fix CPartialMerkleTree.nTransactions signedness":
(https://github.com/bitcoin/bitcoin/pull/29363#issuecomment-1924312018)
ACK

💬 jonatack commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1476415646)
Adding a new configuration option would need a release note.

💬 knst commented on pull request "refactor: Remove excess reserve() call for SecureString":
(https://github.com/bitcoin/bitcoin/pull/29364#issuecomment-1924351566)
@Empact

In the original 2011 implementation, a regular std::string was used for the passphrase:
```
commit b7bcaf940d27fa8cfe89422943fbeaab7a350930
Author: Wladimir J. van der Laan <laanwj@gmail.com>
Date: Wed Aug 24 22:07:26 2011 +0200

Wallet encryption part 2: ask passphrase when needed, add menu options
diff --git a/src/qt/askpassphrasedialog.cpp b/src/qt/askpassphrasedialog.cpp
new file mode 100644
index 0000000000..a297513a62
--- /dev/null
+++ b/src/qt/askpassphrasedia
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1924360021)
Added two commits to stay in sync with SRI: https://github.com/stratum-mining/stratum/pull/742

Next week I'll try to rebase this on the latest #29346, which has diverged a bit.

💬 jonatack commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476464485)
Default value provided twice, can omit second one.

`3. maxburnamount (numeric or string, optional, default="0.00") Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value(default of 0), expressed in BTC.`


```suggestion
"Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + C
...

🤔 stratospher reviewed a pull request: "test: use v2 everywhere for P2PConnection if --v2transport is enabled"
(https://github.com/bitcoin/bitcoin/pull/29358#pullrequestreview-1859682894)
tested ACK d8165db.

💬 stratospher commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1476463624)
d065b33: maybe say only for `LARGE_REORG_SIZE` tests? because the last 2 tests have v2 peers after [reconnection](https://github.com/bitcoin/bitcoin/blob/d8165db8e889f423351ae35fd375c982e793d136/test/functional/feature_block.py#L1312-L1320) due to block height mismatch/invalid block header version.

💬 stratospher commented on pull request "test: use v2 everywhere for P2PConnection if --v2transport is enabled":
(https://github.com/bitcoin/bitcoin/pull/29358#discussion_r1476295813)
d065b33: nit: ChaCha20 typo here and in feature_block.py

💬 jonatack commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476465530)
Same suggestion here as https://github.com/bitcoin/bitcoin/pull/28950/files#r1476464485.

```suggestion
"Reject transactions with provably unspendable outputs (e.g. 'datacarrier' outputs that use the OP_RETURN opcode) greater than the specified value, expressed in " + CURRENCY_UNIT + ".\n"
```

💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1924395126)
> In that case, I think it's totally fine for you to maintain a separate client in case miners would like to mine your potential future nonstandard transactions, and I wish you the best of luck in this endeavor.

> As you correctly point out, if this PR is merged users and miners have other options to coordinate for transactions which the community writ large doesn't want to have polluting their local mempool. It seems like your objective in the comments section of this PR isn't really to NACK
...

🤔 murchandamus reviewed a pull request: "wallet: Set descriptors flag after migrating blank wallets"
(https://github.com/bitcoin/bitcoin/pull/29367#pullrequestreview-1860014271)
cod review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7

Nit: The code behavior seems correct to me, but I would prefer if either the commit message were adapted to state that the `WALLET_FLAG_DESCRIPTOR` is being set for all successfully migrated wallets, or if the code were adapted to more explicitly implement the behavior described in the commit as described in my comment above.

💬 murchandamus commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476493286)
@ryanofsky Yeah, I was not proposing a change in behavior. It just seems cleaner to tie the intended behavior directly to the relevant wallet property than to a generic boolean like `success` that is instantiated half a code block away and much more likely to change its meaning in the course of the project.

@achow101: I would have expected that `DoMigration` sets `WALLET_FLAG_DESCRIPTOR`, but it doesn’t seem to be the case. If you wanted to set it generally, perhaps you could just set it afte
...

💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1924448996)
ACK a1414b3388a870aeb463e9cc293d47cbd86e7f0e

via rangediff a14b95129d3a2894b7a41ce919a426bb60f62e35..a1414b3388a870aeb463e9cc293d47cbd86e7f0e

The changes since my prior ACK appear to only pertain to improvements in the fuzz test to avoid circular packages and a typo fix on a comment in the code.

📝 mzumsande opened a pull request: "fix intermittent failure in `rpc_setban.py --v2transport`, run it in CI"
(https://github.com/bitcoin/bitcoin/pull/29372)
This test failed for me on master locally:
The reason is that when initiating a v2 connection and being immediately disconnected, a node cannot know if the disconnect happens because the peer only supports v1, or because it has banned you, so it schedules to reconnect with v1. If the test doesn't wait for that, the reconnect can happen at a bad time, resulting in failure in a later `connect_nodes` call.
Also add the test with `--v2transport` to the test runner because banning with v2 seems lik
...

💬 achow101 commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476516869)
`DoMigration` does do that, that's why this is only a problem for blank wallets.

As currently implemented, this only sets it for blank wallets, but I do also think the correct behavior is to set it for all wallets, but as I said earlier, I have not analyzed that yet.