🤔 furszy reviewed a pull request: "descriptors: Be able to specify change and receiving in a single descriptor string"
(https://github.com/bitcoin/bitcoin/pull/22838#pullrequestreview-2226106396)
Almost finish, left a question.

💬 furszy commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1707826379)
Small comment about this (and a self-reminder):
`m_keys` is actually a vector of pubkey providers (a vector of vectors). Each top-level vector corresponds to a key in the expression, and the inner vectors relates to the multipath values. Thus, because there is no possible multipath specifier outcome from `miniscript::FromScript`, the resulting Miniscript is built from the combination of all first terms of each inner vector.

Maybe a comment here and something at the `m_keys` field declaration
...

💬 furszy commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1707839205)
```suggestion
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Descriptor with multipath derivation path specifiers are not allowed");
```

💬 furszy commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1709515326)
In 29825fda8d6eaa:

nano styling nit:
```c++
// Return single descriptor
if (descs.size() == 1) {
return addresses;
}

// Expand multipath descriptor
UniValue ret(UniValue::VARR);
ret.push_back(addresses);
for (size_t i = 1; i < descs.size(); ++i) {
ret.push_back(DeriveAddresses(descs.at(i).get(), range_begin, range_end, key_provider));
}
return ret;
```

💬 furszy commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1709846109)
In 7ca8b5b5c:

This could be the default behavior but when the "internal" argument is provided, I think all descriptors should follow that argument (all internal or all external if the flag is set). Then, on a follow-up, we could make "internal" accept an array, allowing users to precisely determine which descriptor in the multipath is internal and which is not.

💬 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#discussion_r1709912751)
The intended and expected usage is for 2 multipath elements for receiving and change. I think diverging from that for default behavior will be confusing, especially since multipath descriptors are in use by other software and use this pattern. I'm fine with a followup that allows the user to override that and specify which should be internal.

💬 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#discussion_r1709913875)
I've updated the comment for `m_keys` to hopefully make this clearer.

💬 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#discussion_r1709913968)
Done

💬 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#discussion_r1709914075)
Done

💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-2276259183)
> > On Windows, there remains some inconsistency. Running `bitcoin-qt.exe -signet -about` ends with a dialog with an orange-coloured icon.
>
> Let me take a look...


Good catch... trying to get it sorted...

🤔 mzumsande reviewed a pull request: "assumeutxo: Drop block height from metadata"
(https://github.com/bitcoin/bitcoin/pull/30598#pullrequestreview-2228449545)
Concept ACK

The block height should also be removed in the `utxo_snapshot` fuzz target.

💬 chrisguida commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-2276303387)
Hmm, odd choice to leave out even the ability to filter out potentially abusive txs.

But sure, I guess we've started a process where Knots is the client that gives users a choice over what to relay, whereas Core just forces them to relay certain things even if they don't want to.

Seems like a confusing path for Core to want to head down, but maybe mempool policy isn't something that should be bundled with Core anyway.

👍 TheCharlatan approved a pull request: "guix: bump time-machine to 7bf1d7aeaffba15c4f680f93ae88fbef25427252"
(https://github.com/bitcoin/bitcoin/pull/30609#pullrequestreview-2228473719)
Nice, ACK eca20bead2daec4898ecf608cdeb74a73766a5f4

Guix builds (aarch64):
```
d079858fb1bc526217ee06f312d97a56c34986440e5f9e108af66eaecacea073 guix-build-eca20bead2da/output/aarch64-linux-gnu/SHA256SUMS.part
2db780ffe39210a3ba113e52362d94840449218ac1747e3a3484606cc36acead guix-build-eca20bead2da/output/aarch64-linux-gnu/bitcoin-eca20bead2da-aarch64-linux-gnu-debug.tar.gz
b56b602bd87e73b11a6b68147c52c6dfa53f0ec4bac52ac749765025e7b43bc9 guix-build-eca20bead2da/output/aarch64-linux-gnu/b
...

💬 achow101 commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2276350075)
Looks like there's a silent merge conflict:

```
kernel/chainparams.cpp: In constructor ‘CMainParams::CMainParams()’:
kernel/chainparams.cpp:194:9: error: no match for ‘operator=’ (operand types are ‘std::__debug::vector<AssumeutxoData>’ and ‘<brace-enclosed initializer list>’)
194 | };
| ^
In file included from /usr/include/c++/11/vector:76,
from /usr/include/c++/11/functional:62,
from /usr/include/c++/11/pstl/glue_algorithm_def
...

💬 theStack commented on pull request "assumeutxo: Drop block height from metadata":
(https://github.com/bitcoin/bitcoin/pull/30598#issuecomment-2276350922)
Concept ACK, now that https://github.com/bitcoin/bitcoin/pull/30516 is history.

While I still think having the block height in the meta-data could have potentially been useful for external tooling, not having it is also not terrible: it will be very likely somehow included in the filename of popular distributed snapshots (so users can at least see it), and for tools that don't have access to the block index, it can at least be implicitly determined by tracking the maximum of all UTXO entries'
...

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2276354835)
Rebased after testnet4 landed. Add the two issues to the PR description.

📝 sipa opened a pull request: "validation: do not wipe utxo cache for stats/scans/snapshots"
(https://github.com/bitcoin/bitcoin/pull/30610)
Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in cache the contents of the cache is still useful.

Split the `FlushStateMode::ALWAYS` mode into a `FORCE_SYNC` (non-wiping) and a `FORCE_FLUSH` (wiping), and then use the former in `scantxoutset`, `gettxoutsetinfo`, and in the currently-unused `CreateUTXOSnapsh
...

💬 sipa commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/30610#discussion_r1710008795)
It is unclear to me what the desired behavior is here, so I opted not to change it, but opinions welcome.

💬 Sjors commented on pull request "validation: assumeutxo params mainnet":
(https://github.com/bitcoin/bitcoin/pull/28553#issuecomment-2276376412)
Rebased due to #30560 and replaced `uint256S("0x...` with `uint256{"...`, as well as using `consteval_ctor` for the block hash, and `m_chain_tx_count`.

🤔 murchandamus reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2225335817)
ReACK 3b6ac646ffea930ae7d8c5d93acaba3587cc9dcd