💬 glozow commented on pull request "RPC: Add maxfeerate and maxburnamount args to submitpackage":
(https://github.com/bitcoin/bitcoin/pull/28950#discussion_r1476287480)
random newline?

💬 maflcko commented on pull request "refactor: Allow CScript construction from any std::input_iterator":
(https://github.com/bitcoin/bitcoin/pull/29369#issuecomment-1924238244)
Yes, either use a newer GCC, or remove the `value_type` type, as in C++20 it is expected to be derived from `element_type` via `std::remove_cv_t<element_type>`.

👍 ryanofsky approved a pull request: "wallet: Set descriptors flag after migrating blank wallets"
(https://github.com/bitcoin/bitcoin/pull/29367#pullrequestreview-1859682311)
Code review ACK 3904123da954f9ebd905de4129aec9f9bab9efc7

💬 ryanofsky commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#discussion_r1476295451)
> I’d be more comfortable if the setting of the flag were conditional on a wallet being blank instead of success being true

Just to be clear, these two conditions should be exactly equivalent due to the `success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);` line so should be a style change, not a change in behavior. I agree the suggestion looks a little better, but the style of dealing with the `success` variable seems here to follow a particular pattern, so would leave it up to
...

📝 Christewart opened a pull request: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371)
This PR adds a `leaf_version` parameter to `taproot_tree_helper()`. Previously the leaf version was hard coded, because we only currently support 1 leaf version.

Proposed soft forks such as #29221 require new leaf versions to be allocated. This PR allows the test framework to accomodate new leaf versions in the test framework.

💬 maflcko commented on pull request "assumeutxo: Get rid of faked nTx and nChainTx values":
(https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1924254747)
> Backwards compatibility code in LoadBlockIndex to override `nTx == 1` values set by previous code when they are fake (when ` (pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`)

Not sure. This can only happen on test-chains, so I think it would be cleaner to just require the developers, if there are any, to finish their background sync, or to wipe their chainstate, or the blocksdir, or the whole test data dir.

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1924257982)
> Might be worth revisiting this now that the cache is much simpler. The signing stuff could be a follow-up PR, but if it's simple enough it would be really nice to include it here. Otherwise, someone using migrate wallet could still hit a wall even with the cache when trying to sign a transaction.

I don't think it's possible to do caching for signing in a sane way that doesn't involve rewriting how we manage keys. The crux of the issue is that we will sign for things even if the wallet has o
...

💬 epiccurious commented on pull request "wallet: Set descriptors flag after migrating blank wallets":
(https://github.com/bitcoin/bitcoin/pull/29367#issuecomment-1924263560)
Tested ACK.

💬 glozow commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1476296848)
Instead of the 021ecfea062151bce0d67bf1575fe7e5e1ee22db approach, I actually think `must contain between 2 and MAX_PACKAGE_COUNT transactions` would be a more helpful interface for users

💬 glozow commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1476310366)
I don't really understand why b2fb55cabc76d842c58b51ff9c64126e6639d1bb is better - I'd interpret this as an array of literal strings "rawtx1", "rawtx2" ?

📝 maflcko converted_to_draft a pull request: "refactor: Allow CScript construction from any std::input_iterator"
(https://github.com/bitcoin/bitcoin/pull/29369)
Currently only (pre)vector iterators and raw pointers are accepted. However, this makes it harder to construct from input iterators provided by other classes, such as `std::span`.

Fix that.

💬 glozow commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1476313442)
thanks for adding a test :+1:

💬 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?