💬 maflcko commented on pull request "cmake: Restrict MSVC-specific workaround to affected versions":
(https://github.com/bitcoin/bitcoin/pull/32499#issuecomment-2880645437)
> Do we really need to force Windows developers to update their toolchains in this case?
I expect the number of Windows devs is still small enough to just ping and ask them, when the time has come.
(https://github.com/bitcoin/bitcoin/pull/32499#issuecomment-2880645437)
> Do we really need to force Windows developers to update their toolchains in this case?
I expect the number of Windows devs is still small enough to just ping and ask them, when the time has come.
💬 Kixunil commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2089248938)
Good that finally someone commented. I think that this should go in line with the wishes of limit-promoters so if adding zero amount is undesirable it should be changed.
> Don't forget the scriptPubKey lengths.
IIUC these are accounted for by the `size` method.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2089248938)
Good that finally someone commented. I think that this should go in line with the wishes of limit-promoters so if adding zero amount is undesirable it should be changed.
> Don't forget the scriptPubKey lengths.
IIUC these are accounted for by the `size` method.
💬 instagibbs commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2089279588)
@Kixunil the serialized size isn't included, but it was also not included in master. You could say it's a bug in the current code as well, but my intention here is to do no worse from payload perspective.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2089279588)
@Kixunil the serialized size isn't included, but it was also not included in master. You could say it's a bug in the current code as well, but my intention here is to do no worse from payload perspective.
👍 ryanofsky approved a pull request: "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner"
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2840779063)
Code review ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f. Lots of suggest suggest changes made since last review, altering function names and signatures and also adding new commit to drop negative fee handling. I like the idea of making the wait function return a BlockRef, that is clearer than what I suggested. Left some comments below but they are not important and this looks good as-is
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2840779063)
Code review ACK 62fc42d475df4f23bd93313f95ee7b7eb0d4683f. Lots of suggest suggest changes made since last review, altering function names and signatures and also adding new commit to drop negative fee handling. I like the idea of making the wait function return a BlockRef, that is clearer than what I suggested. Left some comments below but they are not important and this looks good as-is
💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089293653)
In commit "interfaces: remove redundant coinbase fee check in `waitNext`" (02d4bc776bbe002ee624ec2c09d7c3f981be1b17)
Might be help to mention #31897 in the commit description to clarify what "now does not include" refers to
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089293653)
In commit "interfaces: remove redundant coinbase fee check in `waitNext`" (02d4bc776bbe002ee624ec2c09d7c3f981be1b17)
Might be help to mention #31897 in the commit description to clarify what "now does not include" refers to
💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089312337)
In commit "interfaces: refactor: move `waitTipChanged` implementation to miner" (62fc42d475df4f23bd93313f95ee7b7eb0d4683f)
Not sure, but I think it might be a good idea to drop the 4th commit c39ca9d4f7bc9ca155692ac949be2e61c0598a97 and instead change the 5th commit 62fc42d475df4f23bd93313f95ee7b7eb0d4683f to use the `kernel_notifications.TipBlock()` consistently instead switching to `chainman.ActiveChain().Tip()` at the end:
<details><summary>diff</summary>
<p>
```diff
--- a/src/no
...
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089312337)
In commit "interfaces: refactor: move `waitTipChanged` implementation to miner" (62fc42d475df4f23bd93313f95ee7b7eb0d4683f)
Not sure, but I think it might be a good idea to drop the 4th commit c39ca9d4f7bc9ca155692ac949be2e61c0598a97 and instead change the 5th commit 62fc42d475df4f23bd93313f95ee7b7eb0d4683f to use the `kernel_notifications.TipBlock()` consistently instead switching to `chainman.ActiveChain().Tip()` at the end:
<details><summary>diff</summary>
<p>
```diff
--- a/src/no
...
💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089303383)
In commit "interfaces: refactor: move `waitNext` implementation to miner" (720f201e652885b9e0aec8e62a1bf9590052b320)
Would be good to change `const std::unique_ptr<CBlockTemplate>& block_template` to just `const CBlockTemplate& block_template` since it doesn't look like it's allowed to be null.
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089303383)
In commit "interfaces: refactor: move `waitNext` implementation to miner" (720f201e652885b9e0aec8e62a1bf9590052b320)
Would be good to change `const std::unique_ptr<CBlockTemplate>& block_template` to just `const CBlockTemplate& block_template` since it doesn't look like it's allowed to be null.
💬 hebasto commented on pull request "cmake: Restrict MSVC-specific workaround to affected versions":
(https://github.com/bitcoin/bitcoin/pull/32499#issuecomment-2880953207)
> > Do we really need to force Windows developers to update their toolchains in this case?
>
> I expect the number of Windows devs is still small enough to just ping and ask them, when the time has come.
Friendly ping @davidgumberg @hodlinator @sipsorcery :)
(https://github.com/bitcoin/bitcoin/pull/32499#issuecomment-2880953207)
> > Do we really need to force Windows developers to update their toolchains in this case?
>
> I expect the number of Windows devs is still small enough to just ping and ask them, when the time has come.
Friendly ping @davidgumberg @hodlinator @sipsorcery :)
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2880958368)
Rebased to include the new comment added in #31624.
The tests I added don't cover the difference in sigops counting. Suggestions are welcome, it could look for "CheckBlock failed" vs "ConnectBlock failed".
(https://github.com/bitcoin/bitcoin/pull/31981#issuecomment-2880958368)
Rebased to include the new comment added in #31624.
The tests I added don't cover the difference in sigops counting. Suggestions are welcome, it could look for "CheckBlock failed" vs "ConnectBlock failed".
💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2089388056)
I rebased #31981 after this was merged and suggested one way we could add test coverage for it.
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2089388056)
I rebased #31981 after this was merged and suggested one way we could add test coverage for it.
💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089392199)
As long as getTip() is in the interface it seems consistent with the rest of this PR to move the implementation, so no need to drop c39ca9d4f7bc9ca155692ac949be2e61c0598a97?
Meanwhile you suggestion can be still be used in the 5th commit.
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089392199)
As long as getTip() is in the interface it seems consistent with the rest of this PR to move the implementation, so no need to drop c39ca9d4f7bc9ca155692ac949be2e61c0598a97?
Meanwhile you suggestion can be still be used in the 5th commit.
💬 vasild commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2089410867)
Bikeshed, feel free to ignore.
It is redundant to have "http" in both the namespace and in the class name: `http_bitcoin::HTTPHeaders::Find()`. Also no need to suffix anything with `_bitcoin` - if we would do that, then we would have to have e.g. `namespace common_bitcoin`, `namespace i2p_bitcoin`, `namespace node_bitcoin` etc. So `http_bitcoin::HTTPHeaders::Find()` could be shortened to `http::Headers::Find()`.
(https://github.com/bitcoin/bitcoin/pull/32061#discussion_r2089410867)
Bikeshed, feel free to ignore.
It is redundant to have "http" in both the namespace and in the class name: `http_bitcoin::HTTPHeaders::Find()`. Also no need to suffix anything with `_bitcoin` - if we would do that, then we would have to have e.g. `namespace common_bitcoin`, `namespace i2p_bitcoin`, `namespace node_bitcoin` etc. So `http_bitcoin::HTTPHeaders::Find()` could be shortened to `http::Headers::Find()`.
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089411844)
yeah it is not, I will do if I have to retouch.
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089411844)
yeah it is not, I will do if I have to retouch.
💬 ismaelsadeeq commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089413630)
I will do if I will retouch as well.
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089413630)
I will do if I will retouch as well.
💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089419101)
re: https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089392199
> As long as getTip() is in the interface it seems consistent with the rest of this PR to move the implementation, so no need to drop [c39ca9d](https://github.com/bitcoin/bitcoin/commit/c39ca9d4f7bc9ca155692ac949be2e61c0598a97)?
Yes that's fine since it's possible the function could be used other places. I just suggested dropping the commit to shrink the PR since getTip method is already pretty simple and doesn't nece
...
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089419101)
re: https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2089392199
> As long as getTip() is in the interface it seems consistent with the rest of this PR to move the implementation, so no need to drop [c39ca9d](https://github.com/bitcoin/bitcoin/commit/c39ca9d4f7bc9ca155692ac949be2e61c0598a97)?
Yes that's fine since it's possible the function could be used other places. I just suggested dropping the commit to shrink the PR since getTip method is already pretty simple and doesn't nece
...
📝 BrandonOdiwuor opened a pull request: "RPC: removeprunedfunds should take an array of txids"
(https://github.com/bitcoin/bitcoin/pull/32501)
Fixes https://github.com/bitcoin/bitcoin/issues/29466
Update `removeprunedfunds` RPC to take an `array of strings of txids` instead of a `single txid string` to allow batch removal of transactions
(https://github.com/bitcoin/bitcoin/pull/32501)
Fixes https://github.com/bitcoin/bitcoin/issues/29466
Update `removeprunedfunds` RPC to take an `array of strings of txids` instead of a `single txid string` to allow batch removal of transactions
✅ maflcko closed an issue: "test: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=32090)"
(https://github.com/bitcoin/bitcoin/issues/30764)
(https://github.com/bitcoin/bitcoin/issues/30764)
💬 maflcko commented on issue "test: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=32090)":
(https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2881049570)
> It happens since [fa38664](https://github.com/bitcoin/bitcoin/commit/fa386642b4dfd88f74488c288c7886494d69f4ed), which added a missing lock on cs_main.
It is fixed since e758b26b85da27ef44f3d2c924f3f08e8c1f4fdf, which I can't explain. But this was a false positive anyway.
The fixed commit is using cmake and I used the command on a fresh install of Ubuntu 24.04:
`rm -rf ./bld-cmake/ && cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DBUILD_FUZZ_BINARY=OFF -DB
...
(https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2881049570)
> It happens since [fa38664](https://github.com/bitcoin/bitcoin/commit/fa386642b4dfd88f74488c288c7886494d69f4ed), which added a missing lock on cs_main.
It is fixed since e758b26b85da27ef44f3d2c924f3f08e8c1f4fdf, which I can't explain. But this was a false positive anyway.
The fixed commit is using cmake and I used the command on a fresh install of Ubuntu 24.04:
`rm -rf ./bld-cmake/ && cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DBUILD_FUZZ_BINARY=OFF -DB
...
💬 maflcko commented on issue "test: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=32090)":
(https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2881055089)
> I might be seeing this today as well: https://gist.github.com/pinheadmz/08758d268ec1741afc8c4f181c0e97d2
>
> My branch forks off of master at [db36a92](https://github.com/bitcoin/bitcoin/commit/db36a92c02b83f2e6477a5a55fc061319f7cc6a3)
I don't think this is related. It looks like some kind of BDB issue. However, this issue is about a different trace, see https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2321313037.
Your issue should be fixed as well, now that BDB is removed.
If
...
(https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2881055089)
> I might be seeing this today as well: https://gist.github.com/pinheadmz/08758d268ec1741afc8c4f181c0e97d2
>
> My branch forks off of master at [db36a92](https://github.com/bitcoin/bitcoin/commit/db36a92c02b83f2e6477a5a55fc061319f7cc6a3)
I don't think this is related. It looks like some kind of BDB issue. However, this issue is about a different trace, see https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2321313037.
Your issue should be fixed as well, now that BDB is removed.
If
...
👍 theuni approved a pull request: "refactor: Remove UB in prevector reverse iterators"
(https://github.com/bitcoin/bitcoin/pull/32490#pullrequestreview-2841063795)
utACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
I didn't test for the UB, the argument for removal is good enough for me.
```c++
prevector<42, char> foo;
std::ranges::reverse_view rv{foo};
```
^^ works
(https://github.com/bitcoin/bitcoin/pull/32490#pullrequestreview-2841063795)
utACK faf9082a5f689e2e51a474bf654e4e9b6ca29685
I didn't test for the UB, the argument for removal is good enough for me.
```c++
prevector<42, char> foo;
std::ranges::reverse_view rv{foo};
```
^^ works