💬 yancyribbens commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3115059948)
Oh I see, SRD generates a different solution each time in the test-suit. That's annoying.
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3115059948)
Oh I see, SRD generates a different solution each time in the test-suit. That's annoying.
🤔 mzumsande reviewed a pull request: "Cache m_cached_finished_ibd where SetTip is called."
(https://github.com/bitcoin/bitcoin/pull/32885#pullrequestreview-3053316434)
> The function is only interesting when it can latch to the IBD finished state.
>
> That's only possible when all four conditions are met, which can only happen when the tip is updated.
>
> The final time based condition can only change when the tip changes as it gets further away with time, not closer.
I think that @luke-jr is right. If we reindex, we set `m_importing` to true in `ImportBlocks`, so any blocks we connect there can never result in getting out of IBD due to the `m_blockma
...
(https://github.com/bitcoin/bitcoin/pull/32885#pullrequestreview-3053316434)
> The function is only interesting when it can latch to the IBD finished state.
>
> That's only possible when all four conditions are met, which can only happen when the tip is updated.
>
> The final time based condition can only change when the tip changes as it gets further away with time, not closer.
I think that @luke-jr is right. If we reindex, we set `m_importing` to true in `ImportBlocks`, so any blocks we connect there can never result in getting out of IBD due to the `m_blockma
...
💬 mzumsande commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2229609944)
since the function is annotated with `EXCLUSIVE_LOCKS_REQUIRED(cs_main)` anyway, why not put it to the beginning of the function, as it is done in most other places?
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2229609944)
since the function is annotated with `EXCLUSIVE_LOCKS_REQUIRED(cs_main)` anyway, why not put it to the beginning of the function, as it is done in most other places?
💬 mzumsande commented on pull request "Cache m_cached_finished_ibd where SetTip is called.":
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2229577797)
First I thought this was not necessary because disconnecting a block shouldn't usually get you out of IBD, but it guess there are edge cases (starting up, with the old tip having a lower timestamp than it's parent block) where this could lead to get us out of IBD?!
(https://github.com/bitcoin/bitcoin/pull/32885#discussion_r2229577797)
First I thought this was not necessary because disconnecting a block shouldn't usually get you out of IBD, but it guess there are edge cases (starting up, with the old tip having a lower timestamp than it's parent block) where this could lead to get us out of IBD?!
💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646189)
Done
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646189)
Done
💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646353)
Made `void`
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646353)
Made `void`
💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646469)
Done
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646469)
Done
💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646537)
Done
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646537)
Done
💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646683)
I think leaving them here is fine.
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646683)
I think leaving them here is fine.
💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646764)
Done
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646764)
Done
💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646853)
Done
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646853)
Done
💬 achow101 commented on pull request "wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively":
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646929)
Done
(https://github.com/bitcoin/bitcoin/pull/28333#discussion_r2229646929)
Done
💬 achow101 commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3115094134)
ACK ec0b90e40e21ccb97c57373f7a7665ab9554acec
In a sample size of 1, this seems to provide a significant improvement in IBD time. It's surprising to me that it looks like this would have had benefit around the time that Taproot activated as previous measurements hadn't indicated that.
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3115094134)
ACK ec0b90e40e21ccb97c57373f7a7665ab9554acec
In a sample size of 1, this seems to provide a significant improvement in IBD time. It's surprising to me that it looks like this would have had benefit around the time that Taproot activated as previous measurements hadn't indicated that.
💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229667232)
I've pulled in your suggestion here.
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229667232)
I've pulled in your suggestion here.
💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229670245)
Not sure if it makes a measurable difference in performance, but I'm happy to use your version. Updated.
(https://github.com/bitcoin/bitcoin/pull/31886#discussion_r2229670245)
Not sure if it makes a measurable difference in performance, but I'm happy to use your version. Updated.
💬 jonatack commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3115124995)
Updated per review feedback (thanks!)
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3115124995)
Updated per review feedback (thanks!)
💬 achow101 commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115210492)
Also seeing @mzumsande's observation if I add the addresses to loopback, but not if I add them to a different interface.
I think it's reasonable to filter out all loopback addresses, so the resolution is probably to just update the instructions to have the addresses bound to a non-loopback interface.
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115210492)
Also seeing @mzumsande's observation if I add the addresses to loopback, but not if I add them to a different interface.
I think it's reasonable to filter out all loopback addresses, so the resolution is probably to just update the instructions to have the addresses bound to a non-loopback interface.
💬 monlovesmango commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3115217031)
My feedback is mostly a lot of annoying nits mainly from leftover variables and comments still referring to old max announcements DoS scoring. I compiled them in my own [branch](https://github.com/monlovesmango/bitcoin/tree/pr32941-TxOrphanage-revamp-cleanups) so you can just cherry pick whatever you think is actually worth taking. Overview below.
- [doc: Fixes GetChildrenFromSamePeer comments](https://github.com/monlovesmango/bitcoin/commit/2a25c28aec3773bdf9a17148e45ce38ee95a7342) - Updates
...
(https://github.com/bitcoin/bitcoin/pull/32941#issuecomment-3115217031)
My feedback is mostly a lot of annoying nits mainly from leftover variables and comments still referring to old max announcements DoS scoring. I compiled them in my own [branch](https://github.com/monlovesmango/bitcoin/tree/pr32941-TxOrphanage-revamp-cleanups) so you can just cherry pick whatever you think is actually worth taking. Overview below.
- [doc: Fixes GetChildrenFromSamePeer comments](https://github.com/monlovesmango/bitcoin/commit/2a25c28aec3773bdf9a17148e45ce38ee95a7342) - Updates
...
💬 pinheadmz commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115236229)
If @b-l-u-e is going to retouch to update the instructions in the test comment I'd also really like to see those instructions executed in a CI test as well.
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115236229)
If @b-l-u-e is going to retouch to update the instructions in the test comment I'd also really like to see those instructions executed in a CI test as well.
💬 mzumsande commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115248007)
For me it would also ok to leave the resolution of #31336 for a separate PR - but this PR shouldn't claim to solve the issue then.
(https://github.com/bitcoin/bitcoin/pull/32757#issuecomment-3115248007)
For me it would also ok to leave the resolution of #31336 for a separate PR - but this PR shouldn't claim to solve the issue then.