💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1640954007)
> It appeared to be necessary, IIRC.

Yes, it is required. However, adding the annotation will only silence the compiler warning.

`FastRandomContext` is not thread safe, so the lock will actually have to be taken.

Putting an `EXCLUSIVE_LOCKS_REQUIRED` only into the c++ file and leaving it out from the header file will not work, because the compiler has no way to see the annotation outside of the module. For example, it can not be seen from `init.cpp`.

💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2169245079)
Ok, I think I found the bug: https://github.com/bitcoin/bitcoin/pull/29625/files#r1640954007

It is in your code, and not a compiler bug :sweat_smile:

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#issuecomment-2169251574)
> Found a crash in the `mocked_descriptor_parse` fuzz target, to reproduce:
>

Thanks for this @dergoegge I'll fix it

👍 1440000bytes approved a pull request: "chainparams: Add achow101 DNS seeder"
(https://github.com/bitcoin/bitcoin/pull/30007#pullrequestreview-2120313513)
reACK https://github.com/bitcoin/bitcoin/pull/30007/commits/2721d64989c2b2114890586b7efd01ab4b062ca6

👍 rkrux approved a pull request: "test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16"
(https://github.com/bitcoin/bitcoin/pull/28312#pullrequestreview-2120422373)
reACK [5cf0a1f](https://github.com/bitcoin/bitcoin/pull/28312/commits/5cf0a1f230389ef37e0ff65de5fc98394f32f60c)

Re-ran make and all functional tests, both are successful.
Thanks @theStack for catching this.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2169520624)
> Ok, I think I found the bug: https://github.com/bitcoin/bitcoin/pull/29625/files#r1640954007
>
> It is in your code, and not a compiler bug :sweat_smile:

Wow, nice catch, thank you. Will fix next week.

💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1641145353)
Ok, apparently these annotations are not nearly as smart as I assumed they were!

💬 sdaftuar commented on issue "RFC: Assumeutxo and large forks and reorgs":
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2169559245)
Good discussion question, thanks for raising!

> Considering that AssumeUtxo sync is meant to be an optional and temporary optimization, and that large reorgs should be very infrequent, it could also make sense to abandon the AssumeUtxo sync, delete the snapshot chainstate and revert to normal sync as soon as we accept a header on a different chain that has more work than the best header of the snapshot chain.

I think something like this makes more sense than to change how the background sy
...

💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2169622121)
Force push: rebased for small conflict in `src/Makefile.am`, collected fixup commits, no other changes.

💬 brunoerg commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#issuecomment-2169980849)
Concept ACK

💬 theuni commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1641289196)
Lol, I even spent a few hours looking directly at this and trying to figure out if this could be a threading problem.

🤦

I suppose I trusted the annotations so much I didn't even notice that the code didn't take the lock. That's embarrassing!

Looks like [this is our issue](https://github.com/llvm/llvm-project/pull/67520).

I'll give that patched branch a shot next week and see if it turns up anything else in our codebase. Will chime in upstream with a +1 as well (assuming it works).

👋 tdb3's pull request is ready for review: "test: write functional test results to csv"
(https://github.com/bitcoin/bitcoin/pull/30291)

👍 tdb3 approved a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2120845152)
ACK 5666af2c6e4924bf8e358ab5121c88d14cb085df
Great job laying groundwork for Sv2.

Left some comments (mostly nits). Ran unit and functional tests (passed).

Other than running unit/functional tests and exercising test networks (e.g. regtest for `generate` calls), any recommendations on other good ways to exercise these changes? `getblocktemplate()` is a pretty crucial RPC, so being extra paranoid here could be prudent.

May want to update the description, since RPC functions beyond `ge
...

💬 tdb3 commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641440653)
Although extraordinarily improbable, is the value of 0 reserved as an invalid hash (maybe I'm forgetting something)? If not, maybe it's worth updating this function to return something like `std::optional<uint256>` instead?

💬 tdb3 commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641438112)
nit: If a default value for `check_merkle_root` is added to `testBlockValidity()`, this call could be updated as well.

💬 tdb3 commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641492827)
nit: this comment (and the one on 811) still mention `CreateNewBlock` rather than the new `createNewBlock`. Feel free to ignore unless touching this file for something else.

💬 tdb3 commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1641433677)
nit: Since this is an interface, to maximize clarity it might be good to add Doxygen-ey style `@param` and `@returns` statements in the function comment, similar to `processNewBlock()`.
Same for `createNewBlock()`.

(https://www.doxygen.nl/manual/commands.html#cmdparam)

Also, would it make sense to provide a default value for `check_merkle_root` (e.g. default true)? Or do we want users of the interface to explicitly state whether or not they want merkle root checking performed?

👍 hebasto approved a pull request: "Enable clang-tidy checks for self-assignment"
(https://github.com/bitcoin/bitcoin/pull/30234#pullrequestreview-2121224489)
ACK 26a7f70b5d2b1cbfbf03e0b57b9b5ea92dafbb19, I have reviewed the code and it looks OK.

💬 hebasto commented on pull request "upnp: fix build with miniupnpc 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30283#issuecomment-2171268746)
We faced a similar API-breaking change in libevent. See https://github.com/bitcoin/bitcoin/issues/23606.

👍 hebasto approved a pull request: "Revert "contrib: macdeploy: monkey-patch gen-sdk to be deterministic""
(https://github.com/bitcoin/bitcoin/pull/30282#pullrequestreview-2121262101)
ACK b03a45b13e4e33b044cae6c97a6d608f6f3d43f3, I have reviewed the code and it looks OK.