💬 sipa commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709591119)
I don't think so: https://godbolt.org/z/vYr4Tsbde
The implicitly-defined `operator!=` falls back to using `operator==`. Only the implicitly defined `operator<`, `operator>`, `operator<=`, `operator>=` falls back to using `operator<=>`.
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r1709591119)
I don't think so: https://godbolt.org/z/vYr4Tsbde
The implicitly-defined `operator!=` falls back to using `operator==`. Only the implicitly defined `operator<`, `operator>`, `operator<=`, `operator>=` falls back to using `operator<=>`.
💬 jonatack commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1709596999)
> Bitcoin Core had a strong preference to connect to peers that listen on port `8333`. Is this what you mean? That was removed at some point.
Yes, that preference for clearnet connections was removed in https://github.com/bitcoin/bitcoin/pull/23542.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1709596999)
> Bitcoin Core had a strong preference to connect to peers that listen on port `8333`. Is this what you mean? That was removed at some point.
Yes, that preference for clearnet connections was removed in https://github.com/bitcoin/bitcoin/pull/23542.
💬 fjahr commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2275938528)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2275938528)
Concept ACK
💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709617445)
I think this has helped a lot, "clusterlin: avoid recomputing potential set on every split (optimization)" now makes sense to me. Lots of interlocking parts, so having small bite-sizes helps.
Can you motivate why `reconsider_pot` is set as true on inclusion of `anc` and not on exclude?
When `anc` is included, it's included in both `inc` and `pot`, then:
`if (reconsider_pot) consider_inc = pot.transactions - inc.transactions;`
so no part of `anc` will be in `consider_inc`.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709617445)
I think this has helped a lot, "clusterlin: avoid recomputing potential set on every split (optimization)" now makes sense to me. Lots of interlocking parts, so having small bite-sizes helps.
Can you motivate why `reconsider_pot` is set as true on inclusion of `anc` and not on exclude?
When `anc` is included, it's included in both `inc` and `pot`, then:
`if (reconsider_pot) consider_inc = pot.transactions - inc.transactions;`
so no part of `anc` will be in `consider_inc`.
💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709618160)
Will let you decide, there are multiple ways to do this, each would be slightly better in a different scenario.
Whichever produces the cleanest code, choose that :)
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1709618160)
Will let you decide, there are multiple ways to do this, each would be slightly better in a different scenario.
Whichever produces the cleanest code, choose that :)
💬 paplorinc commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275958345)
ACK 855784d3a0026414159acc42fceeb271f8a28133
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275958345)
ACK 855784d3a0026414159acc42fceeb271f8a28133
💬 maflcko commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275970516)
> This PR carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and - potentially - backwards incompatible behaviour change.
>
> After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that.
I think the description can be clarified that there are two user-facing settings that are now str
...
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2275970516)
> This PR carves out the few `uint256S` callsites that may potentially prove a bit more controversial to change because they deal with user input and - potentially - backwards incompatible behaviour change.
>
> After this PR, the remaining work to remove `uint256S` completely is almost entirely mechanical and/or test related. I will open that PR once #30560 is merged because it builds on that.
I think the description can be clarified that there are two user-facing settings that are now str
...
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709211453)
in c68d26a0e97d7544076f00a467a8eca16658077a
If we're meant to avoid jump ahead, should this be gated on `reconsider_pot` as well?
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1709211453)
in c68d26a0e97d7544076f00a467a8eca16658077a
If we're meant to avoid jump ahead, should this be gated on `reconsider_pot` as well?
💬 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