💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709766961)
@instagibbs @glozow Let me try giving an explanation here. If this helps you, I'll rework it into a comment in the code.

Think of the set of transactions in every work items as falling within one of four classes (roughly from "more likely to be included" to "less likely to be included"):
* **I**: definitely included transactions, (= `item.inc`).
* **P**: possibly included, and including any of them will definitely increase the included set's feerate (even when other lower-fee P transactions
...

🤔 tdb3 reviewed a pull request: "doc, chainparams: 29775 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/30604#pullrequestreview-2228232965)
Concept ACK

💬 tdb3 commented on pull request "doc, chainparams: 29775 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1709789971)
Looks like `feature_config_args.py` needs to be updated to reflect the updated log. Something like:

```diff
diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py
index bb20e2baa85..201112ba0d5 100755
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -378,7 +378,7 @@ class ConfArgsTest(BitcoinTestFramework):

def test_testnet3_deprecation_msg(self):
self.log.info("Test testnet3 deprecation
...

💬 tdb3 commented on pull request "doc, chainparams: 29775 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#discussion_r1709771804)
I'm partial to omitting a specific release number (e.g. and maybe including it just in release notes instead), but It's not something I feel super strongly about.

💬 zawy12 commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2276135560)
There's usually a "timewarp" attack if

1. monotonic timestamps are not enforced,
2. timespan limits are used (1/4 & 4x in BTC), and
3. miner has >50% hashrate.

It doesn't depend on the 2015/2016 "hole" in bitcoin's measurement of nActualtimespan.

The difficulty fix seems to still have a hole if nActualtimespan can be negative. Granted, the attack below requires 16 weeks. The fix is an improvement because the 3 conditions above usually require only 2.5 difficulty adjustment windows t
...

💬 fanquake commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709821764)
Was curious how much difference using `[[likely]]` may/may-not have, as I've seen mixed discussions around it's usage. For a single call to `EvaluateDown` (from the profile_estimate pass):


💬 pablomartin4btc commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2276180981)
> > "Migrate Wallet" is shown when there are no wallets available at all.
>
> I think that's ok.

Yeah, it was just an observation (mainly to compare when the menu is disabled), thanks!

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2276184218)
I can't reproduce the CI failure, but hopefully this fixes it: https://github.com/bitcoin/bitcoin/compare/a19acfccee6f118fad2fb9d090157e5c55cd8465..e73740fde107f79eeee8e55c6bed156713abe8a5

See https://github.com/bitcoin/bitcoin/commit/2d28478d0791689070bd23df5b5640e9dae6f786#r145179932

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