👍 itornaza approved a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2131043232)
Code review and std-tests ACK a9716c53f05082d6d89ebea51a46d4404efb12d7

Redirected to this PR from https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2139460402. I really like how clearly the mining interface is layed out after @ryanofsky comment https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2108528652 to further support @Sjors work on integrating the noise protocol that seems to be needed for stratum v2.

I have closely followed the commit history, built and run all sta
...

💬 itornaza commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1647947327)
non-blocking nit: For consistency with the rest of the headers in this file, you may want to convert the tabs after params to spaces.

👍 ryanofsky approved a pull request: "build: add `-Wundef`"
(https://github.com/bitcoin/bitcoin/pull/29876#pullrequestreview-2131081733)
Code review ACK 9e5bd977688f28a29806236f0faa55d5272f5b65

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2181274931)
Rebased after the merge of #30202.

💬 sr-gi commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1647999066)
Rebased

💬 sr-gi commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#issuecomment-2181323382)
Rebased on top of master to cover https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1629164166, which was addressed by #29575

🤔 mzumsande reviewed a pull request: "assumeutxo: Check snapshot base block is not in invalid chain"
(https://github.com/bitcoin/bitcoin/pull/30267#pullrequestreview-2131034995)
Code Review ACK 750f8b50a749e577fc11f2f9f79e06cdd29e66f5

💬 mzumsande commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1647940464)
typo: parents

💬 mzumsande commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1648003009)
This changes it such that the path is no longer included in the RPC error. I think that it wasn't really helpful anyway (the user already provided the path when calling the RPC, so why the need to report it back to the user?), but just wanted to mention it because in cases someone would miss that.

📝 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

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

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

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

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

👋 theuni's pull request is ready for review: "depends: bump miniupnpc to 2.2.8"
(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

💬 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

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

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

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