💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1773732081)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771915747

Rewrote the comment so hopefully it is less confusing now.

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1773727654)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771936209

Added a comment to mining.capnp about getting rid of BlockValidationState here and returning something simpler from testBlockValidity

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1773730480)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1771916488

I rewrote the comment and tweaked implementation of this to avoid use of non-standard Span class, so this could be dropped and moved to libmultiprocess in the future. But leaving that for a followup since it will require other changes to libmultiprocess.

💬 brunoerg commented on pull request "[tests] New fuzz target wallet_rpc":
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2371885788)
What is the state of this?

🤔 mzumsande reviewed a pull request: "test: Add missing sync_mempools() to fill_mempool()"
(https://github.com/bitcoin/bitcoin/pull/30948#pullrequestreview-2325964327)
Tested ACK faf801515f8fcd11a3454105cab66c38f6f240fe

📝 mzumsande opened a pull request: "validation: Disable CheckForkWarningConditions for background chainstate"
(https://github.com/bitcoin/bitcoin/pull/30962)
The comparison of `m_best_invalid` with the tip of the chainstate makes no sense for the background chainstate and can lead to incorrect log messages.

Fixes #30958

💬 mzumsande commented on issue "`invalidateblock` during background validation":
(https://github.com/bitcoin/bitcoin/issues/30958#issuecomment-2372000480)
I agree that these messages make no sense and think that the check should just be skipped for the background chainstate, see #30962.

💬 ismaelsadeeq commented on pull request "test: Add missing sync_mempools() to fill_mempool()":
(https://github.com/bitcoin/bitcoin/pull/30948#discussion_r1773865111)
Thanks, by the way this was Tested ACK faf801515f8fcd11a3454105cab66c38f6f240fe

🤔 ismaelsadeeq reviewed a pull request: "Cluster linearization: separate tests from tests-of-tests"
(https://github.com/bitcoin/bitcoin/pull/30605#pullrequestreview-2326163992)
Concept ACK

👍 TheCharlatan approved a pull request: "validation: Disable CheckForkWarningConditions for background chainstate"
(https://github.com/bitcoin/bitcoin/pull/30962#pullrequestreview-2326305786)
ACK c0a0c72b4d68a4f0c53c2c4b95f4d6e399f8e4ee

💬 theuni commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1774001261)
This needs a `GUARDED_BY(m_tip_block_mutex)`.

Adding it causes new warnings in `validation_chainstate_tests.cpp`. For ex:

>
> src/test/validation_chainstate_tests.cpp:73:46: warning: reading variable 'm_tip_block' requires holding mutex 'm_node.notifications->m_tip_block_mutex' [-Wthread-safety-analysis]

I assume those particular accesses are harmless (maybe actually racy in the right conditions?), but future users may not be.

Related: It's a shame that the condvar/mutex leak out o
...

💬 fjahr commented on pull request "validation: Disable CheckForkWarningConditions for background chainstate":
(https://github.com/bitcoin/bitcoin/pull/30962#issuecomment-2372255365)
utACK c0a0c72b4d68a4f0c53c2c4b95f4d6e399f8e4ee

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1774007908)
Gave it another go in the latest push (071342d4c7627200323d8f22cc37ea022b2e9d69) and your direction seems to be working fine. Happy to see the linter changes go!

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1774010008)
(Latest push 071342d4c7627200323d8f22cc37ea022b2e9d69 doesn't touch linter - resolving).

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1774014280)
`constexpr` functions are not guaranteed to be able to evaluate at compile time for all possible parameters, so exercising that path in the test still provides some value IMO.

📝 maflcko opened a pull request: "doc: Adjust links in OSS-Fuzz section"
(https://github.com/bitcoin/bitcoin/pull/30963)
Adjust the links after the google issue tracker migration and replace the remaining paragraph with a link to https://bitcoincore.org/en/security-advisories/

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#issuecomment-2372298030)
(Just added co-authorship of maflcko in latest push from 071342d4c7627200323d8f22cc37ea022b2e9d69 -> a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b).

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1774032120)
```suggestion
{
BOOST_CHECK_THROW(TfmF(fmt.fmt, std::tuple_cat(std::array<int, NumArgs + 1>{})), tfm::format_error);
}
```

style nit?

But let's wait for CI first. There is a good chance that `tuple_cat` doesn't compile on Windows or one of the older compilers :sweat_smile:

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1774062364)
I like parts of your direction here, and have taken the time to try it out in 2fb2e72f45a1c81d036515861713371fec2bbe2c. My aim is to retain the tests of leaving out one arg though (which your suggestion doesn't include). I'm not enough of a variadic template arg magician to figure out how to skip the last argument, which forces me to still lean on maflcko's `tuple_cat` approach, leaving it feeling somewhat half-baked.

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1774076507)
Surely that's more than style? :)

Would be nice to have both -1 and +1. Tried out your suggestion but tinyformat fails to throw in 8 of the cases. For example in `"%12$s 999$s %2$s"` with 13 arguments, only using the 12th is not considered an error.