💬 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.

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1774080909)
Ah right, tinyformat may accept "invalid" input. Feel free to resolve.

📝 arik-so opened a pull request: "Disable RBF Rule 2"
(https://github.com/bitcoin/bitcoin/pull/30964)
One of the current RBF acceptance criteria is the prohibition of new unconfirmed inputs in the replacement transaction. That means that all replacement inputs must either be confirmed, or have unconfirmed spends that would get evicted.

However, that limitation is trivially circumvented. If one wanted to spend an unconfirmed UTXO in the replacement transaction, all one would need to do is create a temporary, independent low-fee-rate transaction that spends that input, and then simply add that
...

📝 TheCharlatan opened a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965)
Before this change the block tree db was needlessly re-opened during startup when loading a completed snapshot. Improve this by letting the block manager open it on construction. This also simplifies the test code a bit.

The change was initially motivated to make it easier for users of the kernel library to instantiate a BlockManager that may be used to read data from disk without loading the block index into a cache.