👍 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)
🤔 ryanofsky reviewed a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2310227385)
Rebased b95bb2179610183d9398d50d8c8fd24b1450ad4d -> f9245e4bb880a6d76bbb994a6e1907c6f8c9f205 ([`pr/mine-types.11`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.11) -> [`pr/mine-types.12`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.11-rebase..pr/mine-types.12)) due to silent conflict with #30440
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2310227385)
Rebased b95bb2179610183d9398d50d8c8fd24b1450ad4d -> f9245e4bb880a6d76bbb994a6e1907c6f8c9f205 ([`pr/mine-types.11`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.11) -> [`pr/mine-types.12`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.12), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.11-rebase..pr/mine-types.12)) due to silent conflict with #30440
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1763530005)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1762668345
> [02c6a1e](https://github.com/bitcoin/bitcoin/commit/02c6a1ed58867fa14b1e0c42f476ef45e64ea7ea) `-> (result: BlockTemplate);`
Thanks, should be updated in latest push.
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1763530005)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1762668345
> [02c6a1e](https://github.com/bitcoin/bitcoin/commit/02c6a1ed58867fa14b1e0c42f476ef45e64ea7ea) `-> (result: BlockTemplate);`
Thanks, should be updated in latest push.
💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1763535883)
See https://github.com/bitcoin/bitcoin/blob/6fc4692797121b54de0c54e5b09ee47f322c038a/src/.clang-format#L23, so I'll leave this as-is for now.
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1763535883)
See https://github.com/bitcoin/bitcoin/blob/6fc4692797121b54de0c54e5b09ee47f322c038a/src/.clang-format#L23, so I'll leave this as-is for now.
💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1763536070)
Ok, done
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1763536070)
Ok, done
💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763544909)
I still think this should be an enum named `EntryState` or something, with all combinations of flags like https://github.com/bitcoin/bitcoin/pull/30906/commits/ba52ebd41673abf63d4495215c84be7244469d30#r1760349038. That's the proper way to do this instead of defining static vars for each value of a char we care about.
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763544909)
I still think this should be an enum named `EntryState` or something, with all combinations of flags like https://github.com/bitcoin/bitcoin/pull/30906/commits/ba52ebd41673abf63d4495215c84be7244469d30#r1760349038. That's the proper way to do this instead of defining static vars for each value of a char we care about.
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763548746)
Absolutely, that's ny plan for tomorrow - but needed to clean up the test before doing that
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1763548746)
Absolutely, that's ny plan for tomorrow - but needed to clean up the test before doing that
💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#issuecomment-2356432684)
> Agree with ryanofsky that it would be good to document rough compile time impact if any.
Sure added a note that it is less than 1% of both memory and time. The time should be offset by dropping the corresponding stuff from the Python linter, which is slower than a compiler at iterating over strings.
(https://github.com/bitcoin/bitcoin/pull/30889#issuecomment-2356432684)
> Agree with ryanofsky that it would be good to document rough compile time impact if any.
Sure added a note that it is less than 1% of both memory and time. The time should be offset by dropping the corresponding stuff from the Python linter, which is slower than a compiler at iterating over strings.