💬 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
(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
(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
(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
...
(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
(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!
(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).
(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.
(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/
(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).
(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:
(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.
(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.
(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.
(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
...
(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.
(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.
💬 hodlinator commented on pull request "DO NOT MERGE: Windows bitcoind stall debugging":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2372386020)
https://github.com/bitcoin/bitcoin/actions/runs/11014291937/job/30584604403?pr=30956#step:10:930
contains:
`bitcoind.vcxproj -> D:\a\bitcoin\bitcoin\build\src\RelWithDebInfo\bitcoind.exe`
...notice the "RelWithDebInfo", so it should be generating .PDB files like it does for me locally.
@maflcko continuing discussion after [your comment in the related issue](https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2326700782):
> Seems fine to integrate in a pull request and then keep t
...
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2372386020)
https://github.com/bitcoin/bitcoin/actions/runs/11014291937/job/30584604403?pr=30956#step:10:930
contains:
`bitcoind.vcxproj -> D:\a\bitcoin\bitcoin\build\src\RelWithDebInfo\bitcoind.exe`
...notice the "RelWithDebInfo", so it should be generating .PDB files like it does for me locally.
@maflcko continuing discussion after [your comment in the related issue](https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2326700782):
> Seems fine to integrate in a pull request and then keep t
...
💬 instagibbs commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2372438645)
here's a link to the first(?) discussion of this rule 2 evasion technique at least I'm aware of: https://github.com/bitcoin/bitcoin/pull/23121#issuecomment-929475999
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2372438645)
here's a link to the first(?) discussion of this rule 2 evasion technique at least I'm aware of: https://github.com/bitcoin/bitcoin/pull/23121#issuecomment-929475999
💬 davidgumberg commented on pull request "DO NOT MERGE: Windows bitcoind stall debugging":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2372441698)
I think you can add something like:
```yml
- uses: actions/upload-artifact@v4
with:
name: dump
path: %RUNNER_TEMP%/**/bitcoind.dmp
if-no-files-found: ignore
```
after the `run:` part of each job.
https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/storing-and-sharing-data-from-a-workflow
https://github.com/actions/upload-artifact#examples
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2372441698)
I think you can add something like:
```yml
- uses: actions/upload-artifact@v4
with:
name: dump
path: %RUNNER_TEMP%/**/bitcoind.dmp
if-no-files-found: ignore
```
after the `run:` part of each job.
https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/storing-and-sharing-data-from-a-workflow
https://github.com/actions/upload-artifact#examples
🤔 theStack reviewed a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2326570451)
I'm not seeing any performance improvements locally with a non-debug build running on an SSD (benchmark shows ~13s both with and without the batching), but still think this PR is important for consistency reasons, i.e. to avoid the new wallet be left in a "half-migrated" state in case anything goes wrong. Changes look good to me at first glance (left only some nits), will do a deeper review round tomorrow.
Might be worth to update the PR description with benchmark results from release builds,
...
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2326570451)
I'm not seeing any performance improvements locally with a non-debug build running on an SSD (benchmark shows ~13s both with and without the batching), but still think this PR is important for consistency reasons, i.e. to avoid the new wallet be left in a "half-migrated" state in case anything goes wrong. Changes look good to me at first glance (left only some nits), will do a deeper review round tomorrow.
Might be worth to update the PR description with benchmark results from release builds,
...