💬 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
...
(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.
(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
...
(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):
(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!
(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
(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.
(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
...
(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");
```
(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;
```
(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.
(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.
(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.
(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
(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
(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...
(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.
(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.
(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
...
(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
...
(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
...