💬 achow101 commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476329208)
I think it should actually unconditionally set the flag, but I haven't fully thought through if that is safe yet. Intuitively, it should be safe since a failed migration would result in the wallet being deleted anyways, but I haven't considered all of the possibilities yet.

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1924278221)
Rebasing for CI

💬 achow101 commented on pull request "refactor: Fix timedata includes":
(https://github.com/bitcoin/bitcoin/pull/29361#issuecomment-1924284209)
ACK fad0fafd5aca699cfab7673f8eb18211139aeb18

💬 jonatack commented on pull request "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses":
(https://github.com/bitcoin/bitcoin/pull/26859#discussion_r1476349061)
Nice, making `BIP155Network` a public enum facilitates updating #27385.

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