🤔 mzumsande reviewed a pull request: "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors"
(https://github.com/bitcoin/bitcoin/pull/30909#pullrequestreview-2532471105)
Code Review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
(https://github.com/bitcoin/bitcoin/pull/30909#pullrequestreview-2532471105)
Code Review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
💬 mzumsande commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1904342279)
Makes sense, I got confused there!
It just seemed a bit unusual, plus we generally use `CHECK_NONFATAL` instead of `assert` in rpc code, which may not be the ideal choice for possible future interface users (though it is currently just used in another rpc call, so it's not an actual problem).
But I'm not an expert on the design of our interfaces, so I don't know if moving it somewhere else is necessary.
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1904342279)
Makes sense, I got confused there!
It just seemed a bit unusual, plus we generally use `CHECK_NONFATAL` instead of `assert` in rpc code, which may not be the ideal choice for possible future interface users (though it is currently just used in another rpc call, so it's not an actual problem).
But I'm not an expert on the design of our interfaces, so I don't know if moving it somewhere else is necessary.
👍 BrandonOdiwuor approved a pull request: "doc: Clarify comments about endianness after #30526"
(https://github.com/bitcoin/bitcoin/pull/31596#pullrequestreview-2532501177)
LGTM ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
(https://github.com/bitcoin/bitcoin/pull/31596#pullrequestreview-2532501177)
LGTM ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
👍 ryanofsky approved a pull request: "rpc: add `revelant_blocks` to `scanblocks status`"
(https://github.com/bitcoin/bitcoin/pull/30713#pullrequestreview-2532399834)
Code review ACK 5b2d0216d871bd481f733506a2e8f80b8a34eca0. Seems like a nice feature, and the implementation is pretty simple. I think there is a minor regression if scanblocks start is called from multiple threads, and suggested a fix below.
re: https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2344018591
> Any ideas on how to slow down `scanblocks()` enough to allow a `status` call to consistently return status of a scan in progress?
Will suggest is a very general solution th
...
(https://github.com/bitcoin/bitcoin/pull/30713#pullrequestreview-2532399834)
Code review ACK 5b2d0216d871bd481f733506a2e8f80b8a34eca0. Seems like a nice feature, and the implementation is pretty simple. I think there is a minor regression if scanblocks start is called from multiple threads, and suggested a fix below.
re: https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2344018591
> Any ideas on how to slow down `scanblocks()` enough to allow a `status` call to consistently return status of a scan in progress?
Will suggest is a very general solution th
...
💬 ryanofsky commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904320752)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Think this is missing a word, suggest "ensure this is _called_ before"
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904320752)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Think this is missing a word, suggest "ensure this is _called_ before"
💬 ryanofsky commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904339347)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Lock order here seems a little subtle. Would be good to have a comment saying it is important to lock this before calling reserve so if a scan is currently happening but is about to finish, an accurate "relevant_blocks" value can be returned before the scanning thread clears it.
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904339347)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Lock order here seems a little subtle. Would be good to have a comment saying it is important to lock this before calling reserve so if a scan is currently happening but is about to finish, an accurate "relevant_blocks" value can be returned before the scanning thread clears it.
💬 ryanofsky commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904332839)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Clearing `relevant_blocks` before checking `g_scanfilter_in_progress` seems not very nice, because it means if scanblocks start is called while another scan is in progress, instead of just returning an "already in progress" error, it will do that but also partially erase the state of the in-progress scan.
This behavior is not just a limitation of the new feature, but also a regression, beca
...
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904332839)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
Clearing `relevant_blocks` before checking `g_scanfilter_in_progress` seems not very nice, because it means if scanblocks start is called while another scan is in progress, instead of just returning an "already in progress" error, it will do that but also partially erase the state of the in-progress scan.
This behavior is not just a limitation of the new feature, but also a regression, beca
...
💬 ryanofsky commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904300535)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
It would be good to declare these next to `g_scanfilter_` variables so all global state is declared in one place, and so other functions besides `scanblocks` can see this information (other RPCs and maybe tests should be able to access it). Would suggest renaming `relevant_blocks` to `g_scanfilter_relevant_blocks` and `cs_relevant_blocks` to `g_scanfilter_relevant_blocks_mutex`. (The "critical
...
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1904300535)
In commit "rpc: add relevant_blocks to scanblocks status" (4c9dc4bbe6b23ae7dba7badbdb6dd6c3d0f74e08)
It would be good to declare these next to `g_scanfilter_` variables so all global state is declared in one place, and so other functions besides `scanblocks` can see this information (other RPCs and maybe tests should be able to access it). Would suggest renaming `relevant_blocks` to `g_scanfilter_relevant_blocks` and `cs_relevant_blocks` to `g_scanfilter_relevant_blocks_mutex`. (The "critical
...
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2573467782)
Updated ad80e410b3bf8814cb5b997715ee8cb28f2a2a66 -> 54bdaaefbc683a61f3051dd90bbaf69de5d6cac5 ([kernel_cache_sizes_5](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_5) -> [kernel_cache_sizes_6](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_5..kernel_cache_sizes_6))
* Addressed @hodlinator's [comment](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1903999989), took sugg
...
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2573467782)
Updated ad80e410b3bf8814cb5b997715ee8cb28f2a2a66 -> 54bdaaefbc683a61f3051dd90bbaf69de5d6cac5 ([kernel_cache_sizes_5](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_5) -> [kernel_cache_sizes_6](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_5..kernel_cache_sizes_6))
* Addressed @hodlinator's [comment](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1903999989), took sugg
...
💬 i-am-yuvi commented on pull request "qa: Improve framework.generate* enforcement (#31403 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31599#issuecomment-2573527165)
> Also, a rebase would be good
Done!
(https://github.com/bitcoin/bitcoin/pull/31599#issuecomment-2573527165)
> Also, a rebase would be good
Done!
💬 i-am-yuvi commented on pull request "func test: Expand tx download preference tests":
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2573560025)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2573560025)
Concept ACK
💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2573570012)
Rebased on the latest #31581.
I added a commit that on testnet3 and testnet4 returns a new template after 20 minutes, to take advantage of the min difficulty rule.
Since `miner_tests.cpp` uses mainnet, I created `testnet4_miner_tests.cpp` to cover this behaviour in the unit tests. A functional test could be added later using the testnet4 blocks added by #31583.
I also switched to using `NodeClock::now()` to be able to use mock time in tests.
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2573570012)
Rebased on the latest #31581.
I added a commit that on testnet3 and testnet4 returns a new template after 20 minutes, to take advantage of the min difficulty rule.
Since `miner_tests.cpp` uses mainnet, I created `testnet4_miner_tests.cpp` to cover this behaviour in the unit tests. A functional test could be added later using the testnet4 blocks added by #31583.
I also switched to using `NodeClock::now()` to be able to use mock time in tests.
💬 1440000bytes commented on issue "Fake bitcoin core website at the top of duckduckgo":
(https://github.com/bitcoin/bitcoin/issues/31602#issuecomment-2573668394)
I submitted an [abuse report](https://abuse.cloudflare.com/phishing) for this related domains to cloudflare because its being used for DNS. They have forwarded the report to the responsible hosting provider (IQWeb FZ-LLC).
I see a warning now when I open this link in the browser:
(https://github.com/bitcoin/bitcoin/issues/31602#issuecomment-2573668394)
I submitted an [abuse report](https://abuse.cloudflare.com/phishing) for this related domains to cloudflare because its being used for DNS. They have forwarded the report to the responsible hosting provider (IQWeb FZ-LLC).
I see a warning now when I open this link in the browser:
💬 theuni commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2573690501)
I'd say this actually isn't a special case, rather it's fixing a logical bug in the current impl.
Currently, we essentially do: `echo "$CMAKE_CXX_FLAGS_RELEASE" | sed "s/-O3/-O2/"`.
Instead, I think we only want to do that if we're not overriding the user. Essentially the same as: https://github.com/bitcoin/bitcoin/blob/28.x/configure.ac#L298
So basically the user will always get -O2, unless they change the optims by setting them in the supported/documented variable: `-DCMAKE_CXX_FLAGS_
...
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2573690501)
I'd say this actually isn't a special case, rather it's fixing a logical bug in the current impl.
Currently, we essentially do: `echo "$CMAKE_CXX_FLAGS_RELEASE" | sed "s/-O3/-O2/"`.
Instead, I think we only want to do that if we're not overriding the user. Essentially the same as: https://github.com/bitcoin/bitcoin/blob/28.x/configure.ac#L298
So basically the user will always get -O2, unless they change the optims by setting them in the supported/documented variable: `-DCMAKE_CXX_FLAGS_
...
🤔 ismaelsadeeq reviewed a pull request: "mining: bugfix: Fix duplicate coinbase tx weight reservation"
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2532569808)
Thanks for your feedback @Sjors and your review @TheCharlatan
I've forced pushed to see [diff](https://github.com/bitcoin/bitcoin/compare/cd48e2ccaf24c9594f5987a29e02b924573134ed..ededdd13f3263df08f7878aa2514d3796436cbfb):
1. Add the `-maxcoinbaseweight` option with default as `8000`.
2. Address review comments from @TheCharlatan
(https://github.com/bitcoin/bitcoin/pull/31384#pullrequestreview-2532569808)
Thanks for your feedback @Sjors and your review @TheCharlatan
I've forced pushed to see [diff](https://github.com/bitcoin/bitcoin/compare/cd48e2ccaf24c9594f5987a29e02b924573134ed..ededdd13f3263df08f7878aa2514d3796436cbfb):
1. Add the `-maxcoinbaseweight` option with default as `8000`.
2. Address review comments from @TheCharlatan
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1904497444)
This is gone now.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1904497444)
This is gone now.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1904447541)
done
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1904447541)
done
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1904402308)
> Is there a material difference between this test block and the previous one that actually exercises the default coinbase weight?
There is no any material difference, removed.
> Could more exact weight accounting where the total weight is checked with assert_equal instead of assert_greater_than_or_equal in verify_block_template be useful?
It would be useful, but the reason why I am not is because there is a little disparity between the target weight and the actual weight.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1904402308)
> Is there a material difference between this test block and the previous one that actually exercises the default coinbase weight?
There is no any material difference, removed.
> Could more exact weight accounting where the total weight is checked with assert_equal instead of assert_greater_than_or_equal in verify_block_template be useful?
It would be useful, but the reason why I am not is because there is a little disparity between the target weight and the actual weight.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1904495041)
I think it has.
I've even modified it to also use the correct weight and clarify the log.
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1904495041)
I think it has.
I've even modified it to also use the correct weight and clarify the log.
💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1904497231)
fixed
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1904497231)
fixed