💬 stickies-v commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275982831)
> I think the description can be clarified that there are two user-facing settings that are now stricter checked.
Thanks, added a section on introduced behaviour change.
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275982831)
> I think the description can be clarified that there are two user-facing settings that are now stricter checked.
Thanks, added a section on introduced behaviour change.
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709648591)
No, we *always* want to consider transactions that were missing from `pot`, because those couldn't have been considered for the parent work item. `reconsider_pot` only controls whether we should reconsider the ones *already* in `pot`.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709648591)
No, we *always* want to consider transactions that were missing from `pot`, because those couldn't have been considered for the parent work item. `reconsider_pot` only controls whether we should reconsider the ones *already* in `pot`.
💬 ryanofsky commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet` - (`void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertion 'it.second' failed.`)":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2275991612)
> but how do you link it to this failure?
It seems pretty clear to me the suggested fix in https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2274237242 will prevent this failure because it removes the assert and handles the case where UnloadWalllet is called by more than one thread, instead of aborting. I agree it would be nice to understand more about this failure, though, because maybe there are more problems and this fix isn't sufficient.
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2275991612)
> but how do you link it to this failure?
It seems pretty clear to me the suggested fix in https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2274237242 will prevent this failure because it removes the assert and handles the case where UnloadWalllet is called by more than one thread, instead of aborting. I agree it would be nice to understand more about this failure, though, because maybe there are more problems and this fix isn't sufficient.
💬 maflcko commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet` - (`void wallet::UnloadWallet(std::shared_ptr<CWallet> &&): Assertion 'it.second' failed.`)":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2276001890)
> I think we are missing something here. Probably the assertion failure is a consequence of another issue that triggers a shutdown during the concurrent wallet load/unload.
Yes, the shutdown happens. See for example https://cirrus-ci.com/task/5752995407724544 , which fails without an assert (by accident), but shuts down.
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2276001890)
> I think we are missing something here. Probably the assertion failure is a consequence of another issue that triggers a shutdown during the concurrent wallet load/unload.
Yes, the shutdown happens. See for example https://cirrus-ci.com/task/5752995407724544 , which fails without an assert (by accident), but shuts down.
💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1709670989)
Fixed during rebase.
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1709670989)
Fixed during rebase.
💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1709671154)
Done
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1709671154)
Done
💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1709671373)
Removed
(https://github.com/bitcoin-core/gui/pull/824#discussion_r1709671373)
Removed
💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2276010915)
> Tested [d45ac03](https://github.com/bitcoin-core/gui/commit/d45ac03a89cc500cd461f878f092cf8ec99e7760). The "Migrate Wallet" menu item is still disabled when no wallet is loaded.
Should be fixed now.
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2276010915)
> Tested [d45ac03](https://github.com/bitcoin-core/gui/commit/d45ac03a89cc500cd461f878f092cf8ec99e7760). The "Migrate Wallet" menu item is still disabled when no wallet is loaded.
Should be fixed now.
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2276023703)
:tada: Thanks everyone!
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2276023703)
:tada: Thanks everyone!
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2276094102)
Rebased and I took @itornaza's patch with some changes, see inline comments on https://github.com/bitcoin/bitcoin/commit/2d28478d0791689070bd23df5b5640e9dae6f786.
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2276094102)
Rebased and I took @itornaza's patch with some changes, see inline comments on https://github.com/bitcoin/bitcoin/commit/2d28478d0791689070bd23df5b5640e9dae6f786.
💬 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
...
(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
(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
...
(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
...