💬 brunoerg commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1654445965)
It seems good to me, I'll address it.

💬 brunoerg commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1654446440)
Yes, I can add this verification.

💬 fanquake commented on pull request "contrib: add R(UN)PATH check to ELF symbol-check":
(https://github.com/bitcoin/bitcoin/pull/30312#issuecomment-2191235833)
> performing binary checks after the installation step.

I'll followup with this separately, pre-CMake, given it will be needed there.

🚀 fanquake merged a pull request: "contrib: add R(UN)PATH check to ELF symbol-check"
(https://github.com/bitcoin/bitcoin/pull/30312)

✅ fanquake closed an issue: "bug: verify-binaries/verify.py incorrectly parses version string; gives error or downloads wrong files"
(https://github.com/bitcoin/bitcoin/issues/30145)

🚀 fanquake merged a pull request: "contrib: Fixup verify-binaries OS platform parsing"
(https://github.com/bitcoin/bitcoin/pull/30147)

👍 real-or-random approved a pull request: "Update libsecp256k1 subtree to latest master"
(https://github.com/bitcoin/bitcoin/pull/30334#pullrequestreview-2141273911)
utACK cc58e958f341d2759fbabe5c9d8cc557e17d587f

💬 AngusP commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654485896)
It is also acquired within `miner.testBlockValidity` (and in `miner.createNewBlock`) so maybe is now unnecessary outside the Miner interface?

https://github.com/bitcoin/bitcoin/blob/a9716c53f05082d6d89ebea51a46d4404efb12d7/src/node/interfaces.cpp#L873-L877

💬 TheCharlatan commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1654486212)
Is this lock required?

💬 laanwj commented on pull request "contrib: add R(UN)PATH check to ELF symbol-check":
(https://github.com/bitcoin/bitcoin/pull/30312#issuecomment-2191286545)
Post-merge ACK

👍 Sjors approved a pull request: "Testnet4 including PoW difficulty adjustment fix"
(https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2140918229)
ACK aba504759caa13338aa574a7839d078b989fbf6d

💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1654259706)
e40f06acfd090797f14abdc1768d7b96ae4eeed8: one advantage of having already had some mining is that we can set `nMinimumChainWork` shortly before the release (if only so people can test the headers pre-sync behavior).

💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1654278535)
71ce039c20f78d6454c3f930d1510f7d3e3b8652: two more followup suggestions:
1. Document `CalculateNextWorkRequired` (including that this is only called for retarget blocks) and `GetNextWorkRequired` (called for every block)
2. Add `Assume((pindexLast->nHeight + 1) % params.DifficultyAdjustmentInterval() == 0);` at the top

💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1654288556)
I just noticed that this `wile` code is duplicated from `GetNextWorkRequired`. A helper function might be nice here. The original is inside an `if (params.fPowAllowMinDifficultyBlocks)`, so refactoring there isn't too scary. It makes it more clear that we consider "last non-special-min-difficulty-rules-block" in two places.

Alternatively your remark in the rationale section of the BIP might get rid of this dpulicate code altogether:

> For the block storm fix an alternative fix could have b
...

💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1654523662)
Also, let's add something like this below `// Special difficulty rule for testnet:`:

```
// Note that the first block of an interval can't be min-difficulty.
```

👍 stickies-v approved a pull request: "rest: don't copy data when sending binary response"
(https://github.com/bitcoin/bitcoin/pull/30321#pullrequestreview-2141349767)
ACK 1556d21599a250297d5f20e5249c970340ab08bc

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1654538720)
Probably not, because `BlockAssembler::CreateNewBlock` takes a lock itself. As does `ChainstateManager::ActiveChainstate()`.

I think I initially added this because `generateblock()` in `rpc/mining.cpp` did it.

I'll add a commit to #30335 to drop it.

@maflcko found something similar

💬 Sjors commented on pull request "Introduce Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30200#discussion_r1654546557)
(unsure if that was indeed the reason)

💬 Sjors commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654551558)
I pushed 1f0559e9954253c23af7ee9b49e57eb3d7a45bea to drop it from `createNewBlock`. Will look at the other occurances too.

💬 fanquake commented on issue "ci: feature_proxy failing in MSVC job":
(https://github.com/bitcoin/bitcoin/issues/29090#issuecomment-2191367471)
https://github.com/bitcoin/bitcoin/actions/runs/9677128783/job/26698120754#step:27:647:
```bash
node2 2024-06-26T10:23:18.549103Z [scheduler] [D:\a\bitcoin\bitcoin\src\net.cpp:2328] [DumpAddresses] [net] Flushed 0 addresses to peers.dat 1ms
test 2024-06-26T10:33:29.783000Z TestFramework.node0 (DEBUG): Stopping node
test 2024-06-26T10:33:29.783000Z TestFramework.node1 (DEBUG): Stopping node
test 2024-06-26T10:33:29.783000Z TestFramework.node2 (DEBUG): Stopping node
test 2024-
...