📝 theuni opened a pull request: "refactor: remove extraneous lock annotations from function definitions"
(https://github.com/bitcoin/bitcoin/pull/30316)
These annotations belong in the declarations rather than the definitions. While harmless now, future versions of clang may warn about these.
Discovered these using the upstream WIP: https://github.com/llvm/llvm-project/pull/67520
(https://github.com/bitcoin/bitcoin/pull/30316)
These annotations belong in the declarations rather than the definitions. While harmless now, future versions of clang may warn about these.
Discovered these using the upstream WIP: https://github.com/llvm/llvm-project/pull/67520
💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1648006777)
I used the WIP above to check for other issues like this in our codebase. It only turned up a few false-positives because of duplicate annotations in declarations + definitions. Thankfully there were no cases of them _only_ in definitions. PR'd here: #30316.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1648006777)
I used the WIP above to check for other issues like this in our codebase. It only turned up a few false-positives because of duplicate annotations in declarations + definitions. Thankfully there were no cases of them _only_ in definitions. PR'd here: #30316.
💬 theuni commented on pull request "refactor: remove extraneous lock annotations from function definitions":
(https://github.com/bitcoin/bitcoin/pull/30316#issuecomment-2181336426)
I should've mentioned in the description that all of these already have proper annotations in their corresponding declarations which is why they're safe to remove. But reviewers should obviously double-check that.
(https://github.com/bitcoin/bitcoin/pull/30316#issuecomment-2181336426)
I should've mentioned in the description that all of these already have proper annotations in their corresponding declarations which is why they're safe to remove. But reviewers should obviously double-check that.
🤔 ryanofsky reviewed a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2131112731)
Code review ACK a9716c53f05082d6d89ebea51a46d4404efb12d7 with one minor suggestion in case you update. Only changes since last review were other small changes to the interface.
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2131112731)
Code review ACK a9716c53f05082d6d89ebea51a46d4404efb12d7 with one minor suggestion in case you update. Only changes since last review were other small changes to the interface.
💬 ryanofsky commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1647987176)
In commit "rpc: call TestBlockValidity via miner interface" (d8a3496b5ad27bea4c79ea0344f595cc1b95f0d3)
Not important, but it might be good to move the `state` output parameter last. The reason would be to work with the libmultiprocess code generator in case we want to expose this interface over IPC. The code generator only knows how to parse Cap'n Proto declarations, not C++ code, so when it encounters a Cap'n Proto method declaration with output parameters like:
```capnp
testBlockValidit
...
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1647987176)
In commit "rpc: call TestBlockValidity via miner interface" (d8a3496b5ad27bea4c79ea0344f595cc1b95f0d3)
Not important, but it might be good to move the `state` output parameter last. The reason would be to work with the libmultiprocess code generator in case we want to expose this interface over IPC. The code generator only knows how to parse Cap'n Proto declarations, not C++ code, so when it encounters a Cap'n Proto method declaration with output parameters like:
```capnp
testBlockValidit
...
👋 theuni's pull request is ready for review: "depends: bump miniupnpc to 2.2.8"
(https://github.com/bitcoin/bitcoin/pull/30301)
(https://github.com/bitcoin/bitcoin/pull/30301)
💬 instagibbs commented on pull request "refactor: remove extraneous lock annotations from function definitions":
(https://github.com/bitcoin/bitcoin/pull/30316#issuecomment-2181465407)
ACK 5729dbbb7424d02c5e5bc4f2eb340fdc1c0100b4
manually checked that all these annotations still live in the declarations
(https://github.com/bitcoin/bitcoin/pull/30316#issuecomment-2181465407)
ACK 5729dbbb7424d02c5e5bc4f2eb340fdc1c0100b4
manually checked that all these annotations still live in the declarations
💬 Mazzika1 commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2181500937)
https://github.com/bitcoin-portal/bitcoincom-solidity-swap.git
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2181500937)
https://github.com/bitcoin-portal/bitcoincom-solidity-swap.git
💬 hebasto commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2181534902)
> I actually meant to ask this in #30193, but why do we cache using run-id in the key `${{ github.job }}-ccache-${{ github.run_id }}` ? As we only cache on master, using only `${{ github.job }}-ccache` would make more sense to me; a single rolling cache per job.
>
> When we search for the cache to load we use a "wildcard" restore `restore-keys: ${{ github.job }}-ccache-` (with no run_id).
>
> This would remove some "duplicates", e.g "macos-native-x86_64-ccache-" has 3 cache entries, when i
...
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2181534902)
> I actually meant to ask this in #30193, but why do we cache using run-id in the key `${{ github.job }}-ccache-${{ github.run_id }}` ? As we only cache on master, using only `${{ github.job }}-ccache` would make more sense to me; a single rolling cache per job.
>
> When we search for the cache to load we use a "wildcard" restore `restore-keys: ${{ github.job }}-ccache-` (with no run_id).
>
> This would remove some "duplicates", e.g "macos-native-x86_64-ccache-" has 3 cache entries, when i
...
💬 hebasto commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2181559048)
Here is a summary of the current GHA cache storage usage:
| prefix | size, MB |
|---|---:|
| macos-native-x86_64-ccache | 380 |
| macos-native-x86_64-ccache | 380 |
| macos-native-x86_64-ccache | 380 |
| macos-native-x86_64-ccache | 380 |
| macos-native-x86_64-ccache | 380 |
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2181559048)
Here is a summary of the current GHA cache storage usage:
| prefix | size, MB |
|---|---:|
| macos-native-x86_64-ccache | 380 |
| macos-native-x86_64-ccache | 380 |
| macos-native-x86_64-ccache | 380 |
| macos-native-x86_64-ccache | 380 |
| macos-native-x86_64-ccache | 380 |
💬 hebasto commented on issue "ci: Move more tasks to GHA?":
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2181572660)
Here is a summary of the current GHA cache storage usage:
| prefix | size, MB |
|---|---:|
| macos-native-x86_64-ccache | 380 |
| win64-native-static-qt | 61 |
| win64-native-ccache-installation | 3 |
| win64-native-ccache | 160 |
| win64-native-vcpkg-tools | 5 |
| win64-native-vcpkg-binary | 51 |
Total: 660 MB.
And 10 GB are available.
> Another downside would be that caching depends artefacts and docker images is hard on GHA. So ideally only tasks with `NO_DEPENDS=1` are m
...
(https://github.com/bitcoin/bitcoin/issues/30304#issuecomment-2181572660)
Here is a summary of the current GHA cache storage usage:
| prefix | size, MB |
|---|---:|
| macos-native-x86_64-ccache | 380 |
| win64-native-static-qt | 61 |
| win64-native-ccache-installation | 3 |
| win64-native-ccache | 160 |
| win64-native-vcpkg-tools | 5 |
| win64-native-vcpkg-binary | 51 |
Total: 660 MB.
And 10 GB are available.
> Another downside would be that caching depends artefacts and docker images is hard on GHA. So ideally only tasks with `NO_DEPENDS=1` are m
...
💬 anointedboss1717 commented on pull request "doc: JSON-RPC request Content-Type is application/json":
(https://github.com/bitcoin/bitcoin/pull/30215#issuecomment-2181576338)
29947
(https://github.com/bitcoin/bitcoin/pull/30215#issuecomment-2181576338)
29947
💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2181583153)
> This job has a ccache size limit of 100mb, the MacOS job has a ccache limit of 400m. @hebasto is there something specific to MacOS for why the ccache limit is higher there?
When I adjusted ccache size limits, I picked a size that allowed to populate the cleared cache with no cleanups in the `ccache --show-stats` report. I'm not aware of any peculiarities that make macOS caches bigger than others.
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2181583153)
> This job has a ccache size limit of 100mb, the MacOS job has a ccache limit of 400m. @hebasto is there something specific to MacOS for why the ccache limit is higher there?
When I adjusted ccache size limits, I picked a size that allowed to populate the cleared cache with no cleanups in the `ccache --show-stats` report. I'm not aware of any peculiarities that make macOS caches bigger than others.
💬 hebasto commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2181589998)
> > Maybe wait for the CI to pass, then address the nits to test the created cache?
>
> Oh, I guess only master creates the cache?
>
> ```
> if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true'
> ```
>
> So I guess this can be merged after addressing the nits.
Maybe comment that line out in a temporary "NO MERGE" commit?
(https://github.com/bitcoin/bitcoin/pull/30310#issuecomment-2181589998)
> > Maybe wait for the CI to pass, then address the nits to test the created cache?
>
> Oh, I guess only master creates the cache?
>
> ```
> if: github.event_name != 'pull_request' && steps.ccache-cache.outputs.cache-hit != 'true'
> ```
>
> So I guess this can be merged after addressing the nits.
Maybe comment that line out in a temporary "NO MERGE" commit?
📝 paplorinc opened a pull request: "WIP Optimize SipHash"
(https://github.com/bitcoin/bitcoin/pull/30317)
WIP, once all changes are done, I'll split this into small changes, I want to see first if CI passes for every scenario.
----------
Profiling `./bitcoind -reindex-chainstate -checkblocks=10000` revealed that hashing is a bottleneck:
<img alt="image" src="https://github.com/bitcoin/bitcoin/assets/1841944/8fa91d78-a5f0-4f4e-a281-f3ff1548b536">
The changes make it ~10% faster - while making the code more maintainable as well.
> make -j10 && src/test/test_bitcoin --run_test=hash_tests &
...
(https://github.com/bitcoin/bitcoin/pull/30317)
WIP, once all changes are done, I'll split this into small changes, I want to see first if CI passes for every scenario.
----------
Profiling `./bitcoind -reindex-chainstate -checkblocks=10000` revealed that hashing is a bottleneck:
<img alt="image" src="https://github.com/bitcoin/bitcoin/assets/1841944/8fa91d78-a5f0-4f4e-a281-f3ff1548b536">
The changes make it ~10% faster - while making the code more maintainable as well.
> make -j10 && src/test/test_bitcoin --run_test=hash_tests &
...
👍 tdb3 approved a pull request: "wallet: notify when preset + automatic inputs exceed max weight"
(https://github.com/bitcoin/bitcoin/pull/30309#pullrequestreview-2131483027)
ACK 3f5027626593c7af546d3b53fc658c87bb815348
Helpful addition to help cover edge cases.
Anything that should be added to unit tests?
(https://github.com/bitcoin/bitcoin/pull/30309#pullrequestreview-2131483027)
ACK 3f5027626593c7af546d3b53fc658c87bb815348
Helpful addition to help cover edge cases.
Anything that should be added to unit tests?
💬 tdb3 commented on pull request "wallet: notify when preset + automatic inputs exceed max weight":
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1648208431)
Tested that this would fail if the `return util::Error...` line was commented out in source/wallet/spend.cpp. It did (as expected).
(https://github.com/bitcoin/bitcoin/pull/30309#discussion_r1648208431)
Tested that this would fail if the `return util::Error...` line was commented out in source/wallet/spend.cpp. It did (as expected).
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2181689921)
Added a big description about the fuzzer serialization format for DepGraph objects.
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2181689921)
Added a big description about the fuzzer serialization format for DepGraph objects.
💬 ariard commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2181708713)
> Possibly the best estimate we can get for that is to look at full-RBF
replacements that do not get mined. It's not too uncommon to see, for example,
OpenTimestamp's full-RBF replacements mysteriously get mined at the second or
third highest fee-rate they were propagated at even though multiple minutes
have passed since the higher fee-rate txs were broadcast. I believe that is
evidence of poor propagation to miners on occasion.
This is good information to have. I’ll recall the original
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2181708713)
> Possibly the best estimate we can get for that is to look at full-RBF
replacements that do not get mined. It's not too uncommon to see, for example,
OpenTimestamp's full-RBF replacements mysteriously get mined at the second or
third highest fee-rate they were propagated at even though multiple minutes
have passed since the higher fee-rate txs were broadcast. I believe that is
evidence of poor propagation to miners on occasion.
This is good information to have. I’ll recall the original
...
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2181796855)
> > Possibly the best estimate we can get for that is to look at full-RBF
> > replacements that do not get mined. It's not too uncommon to see, for example,
> > OpenTimestamp's full-RBF replacements mysteriously get mined at the second or
> > third highest fee-rate they were propagated at even though multiple minutes
> > have passed since the higher fee-rate txs were broadcast. I believe that is
> > evidence of poor propagation to miners on occasion.
>
> This is good information to have.
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2181796855)
> > Possibly the best estimate we can get for that is to look at full-RBF
> > replacements that do not get mined. It's not too uncommon to see, for example,
> > OpenTimestamp's full-RBF replacements mysteriously get mined at the second or
> > third highest fee-rate they were propagated at even though multiple minutes
> > have passed since the higher fee-rate txs were broadcast. I believe that is
> > evidence of poor propagation to miners on occasion.
>
> This is good information to have.
...