💬 hebasto commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2356089545)
@theStack
Could rebase once more please to refresh the CI?
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2356089545)
@theStack
Could rebase once more please to refresh the CI?
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2356103199)
> Is this so that re-runs (of the same PR) always hit the cache? If that's it I'm not _totally_ sure it's worth it. My intuition says that if we had multiple servers, even if a re-run took place on a different machine, we'd likely get a _pretty decent_ cache hit from another run or a previous master build
Right. It is unclear whether this is worth it. I think when it comes to builds on master, the speed doesn't matter at all, because no one is waiting for the result to appear immediately. So
...
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2356103199)
> Is this so that re-runs (of the same PR) always hit the cache? If that's it I'm not _totally_ sure it's worth it. My intuition says that if we had multiple servers, even if a re-run took place on a different machine, we'd likely get a _pretty decent_ cache hit from another run or a previous master build
Right. It is unclear whether this is worth it. I think when it comes to builds on master, the speed doesn't matter at all, because no one is waiting for the result to appear immediately. So
...
💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2356181083)
> CPU-bound (and not IO-bound)
Actually, the long-running fuzz test may be IO-bound so using a ramdisk could speed them up. So this would be another idea to look at.
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2356181083)
> CPU-bound (and not IO-bound)
Actually, the long-running fuzz test may be IO-bound so using a ramdisk could speed them up. So this would be another idea to look at.
💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2356184027)
> @theStack
>
> Could rebase once more please to refresh the CI?
Yes, done.
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2356184027)
> @theStack
>
> Could rebase once more please to refresh the CI?
Yes, done.
👍 hebasto approved a pull request: "[28.x] Further backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/30827#pullrequestreview-2310065055)
ACK 06a7df70df30879e0b691d1a252636f703b8cdfb, I've backported the listed PRS locally. The only merge conflict I faced was in https://github.com/bitcoin/bitcoin/pull/30807. It was trivial to resolve.
(https://github.com/bitcoin/bitcoin/pull/30827#pullrequestreview-2310065055)
ACK 06a7df70df30879e0b691d1a252636f703b8cdfb, I've backported the listed PRS locally. The only merge conflict I faced was in https://github.com/bitcoin/bitcoin/pull/30807. It was trivial to resolve.
💬 maflcko commented on pull request "build: speed up by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2356197180)
> It's clear to see that the header generation is self-contained, and afaik that's the only place that would matter for build performance.
Could rebase on current master, now that the header generation is 30x faster, after 2a0949f0977db2dbb7ac452e8d58d413b9526756 ? This should also change the benchmark in the pull description.
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2356197180)
> It's clear to see that the header generation is self-contained, and afaik that's the only place that would matter for build performance.
Could rebase on current master, now that the header generation is 30x faster, after 2a0949f0977db2dbb7ac452e8d58d413b9526756 ? This should also change the benchmark in the pull description.
💬 maflcko commented on pull request "ci: Use `ninja` to build in macOS native CI job":
(https://github.com/bitcoin/bitcoin/pull/30915#issuecomment-2356214347)
review ACK d01b85bfecbcf16ea16f90e2ade7537bf582269f
Can't hurt to test this in one task
(https://github.com/bitcoin/bitcoin/pull/30915#issuecomment-2356214347)
review ACK d01b85bfecbcf16ea16f90e2ade7537bf582269f
Can't hurt to test this in one task
👍 ryanofsky approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2310085862)
Code review ACK 7942951e3fcc58f7db0059546d03be9ea243f1db. Just rebased since last review
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2310085862)
Code review ACK 7942951e3fcc58f7db0059546d03be9ea243f1db. Just rebased since last review
👍 TheCharlatan approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2310089065)
Re-ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2310089065)
Re-ACK 7942951e3fcc58f7db0059546d03be9ea243f1db
💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#issuecomment-2356233258)
Rebased after merge of #30286.
(https://github.com/bitcoin/bitcoin/pull/30605#issuecomment-2356233258)
Rebased after merge of #30286.
💬 0xBEEFCAF3 commented on pull request "CAT in Tapscript (BIP-347)":
(https://github.com/bitcoin/bitcoin/pull/29247#discussion_r1763450844)
Great suggestion! Added some additional unit tests to script_tests.json testing different combinations of scripts with and without (SCRIPT_VERIFY_DISCOURAGE_OP_CAT, SCRIPT_VERIFY_OP_CAT and SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS)
(https://github.com/bitcoin/bitcoin/pull/29247#discussion_r1763450844)
Great suggestion! Added some additional unit tests to script_tests.json testing different combinations of scripts with and without (SCRIPT_VERIFY_DISCOURAGE_OP_CAT, SCRIPT_VERIFY_OP_CAT and SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS)
💬 jaybny commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2356240428)
ok. is C++20 a requirement? why? it's not fully supported by clang or gcc yet. can get 90% there with libstdc++ , but I guess it's only a matter of time.
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2356240428)
ok. is C++20 a requirement? why? it's not fully supported by clang or gcc yet. can get 90% there with libstdc++ , but I guess it's only a matter of time.
💬 hebasto commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2356249815)
> is C++20 a requirement?
Yes. Please refer to https://github.com/bitcoin/bitcoin/pull/28349.
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2356249815)
> is C++20 a requirement?
Yes. Please refer to https://github.com/bitcoin/bitcoin/pull/28349.
💬 mzumsande commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763467219)
I thought about it a bit and don't see a great non-invasive solution to this: We could add a call to `NotifyHeaderTip` in the calling functions after `cs_main` is released, which would be in `ActivateBestChain()` (plus in the `invalidateblock` / `reconsiderblock` rpcs which seem less important), and I'm unsure if we want to refactor this function to always call `NotifyBestHeader()` just for the special case where a connected block turns out to be invalid.
In any case, since `m_best_header` is
...
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763467219)
I thought about it a bit and don't see a great non-invasive solution to this: We could add a call to `NotifyHeaderTip` in the calling functions after `cs_main` is released, which would be in `ActivateBestChain()` (plus in the `invalidateblock` / `reconsiderblock` rpcs which seem less important), and I'm unsure if we want to refactor this function to always call `NotifyBestHeader()` just for the special case where a connected block turns out to be invalid.
In any case, since `m_best_header` is
...
💬 mzumsande commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763468418)
will change to "/" so it mirrors the "invalidation / reconsideration" in the line before.
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763468418)
will change to "/" so it mirrors the "invalidation / reconsideration" in the line before.
💬 mzumsande commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763474241)
change to `::cs_main`
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763474241)
change to `::cs_main`
💬 mzumsande commented on pull request "validation: fix m_best_header tracking and BLOCK_FAILED_CHILD assignment":
(https://github.com/bitcoin/bitcoin/pull/30666#issuecomment-2356297541)
[321097a ](https://github.com/bitcoin/bitcoin/commit/321097a685b67f82c32f53652227b7177fe97a95)to [0bd53d9](https://github.com/bitcoin/bitcoin/commit/0bd53d913c1c2ffd2d0779f01bc51c81537b6992): addressed feedback by @jonatack
>Is this ready for review or should we wait until you have addressed this @mzumsande ?
It's ready for review, see https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763467219 - but of course I'm open to suggestions!
(https://github.com/bitcoin/bitcoin/pull/30666#issuecomment-2356297541)
[321097a ](https://github.com/bitcoin/bitcoin/commit/321097a685b67f82c32f53652227b7177fe97a95)to [0bd53d9](https://github.com/bitcoin/bitcoin/commit/0bd53d913c1c2ffd2d0779f01bc51c81537b6992): addressed feedback by @jonatack
>Is this ready for review or should we wait until you have addressed this @mzumsande ?
It's ready for review, see https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1763467219 - but of course I'm open to suggestions!
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2356328386)
bdfeb12eec31eb885479eed293fe3677cfac52b2 is a really nice simplification.
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2356328386)
bdfeb12eec31eb885479eed293fe3677cfac52b2 is a really nice simplification.
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763508568)
Did a significant restructure, the tabular tests should be a lot easier to comprehend and modify now.
I still need to get rid of remaining flags in the tests, will do that tomorrow - any early feedback until then is appreciated.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763508568)
Did a significant restructure, the tabular tests should be a lot easier to comprehend and modify now.
I still need to get rid of remaining flags in the tests, will do that tomorrow - any early feedback until then is appreciated.
🚀 fanquake merged a pull request: "[28.x] Further backports and rc2"
(https://github.com/bitcoin/bitcoin/pull/30827)
(https://github.com/bitcoin/bitcoin/pull/30827)