💬 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
...
💬 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'
...
(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'
...