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

🚀 fanquake merged a pull request: "rest: don't copy data when sending binary response"
(https://github.com/bitcoin/bitcoin/pull/30321)

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

The commit referenced in this commit message is wrong.

🚀 fanquake merged a pull request: "chainparams: Add achow101 DNS seeder"
(https://github.com/bitcoin/bitcoin/pull/30007)

💬 Sjors commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654603274)
The lock at line 390 has been there from the introduction of the `generateblock` RPC call in #17693. I don't see any discussion of it in the reviews.

I think it's because `assert(pindexPrev && pindexPrev == chainstate.m_chain.Tip());` inside of `TestBlockValidity`. Added d23968e0fb6e1d7c5d19b07722796181d6e04f5b to explain that, and also make `testBlockValidity` require the lock.

📝 willcl-ark opened a pull request: "WIP: Permit Combiner to strip bip32_deriv information"
(https://github.com/bitcoin/bitcoin/pull/30341)
Related to https://github.com/bitcoin/bitcoin/issues/30294

Looking for approach (N)ACK.

Previously setting the `bip32derivs` flag to false with the `walletprocesspsbt` RPC correctly does not include `bip32_derivs` for inputs in the PSBT, but does include `bip32_derivs` for outputs.

User may want to actively strip all bip32 derivation paths from a PSBT for privacy reasons however. It may make sense to do this after signing your inputs and outputs during a manual coinjoin as demonstrated
...

💬 willcl-ark commented on issue "Setting `bip32derivs` to `false` with `walletprocesspsbt` includes `bip32_derivs` for outputs.":
(https://github.com/bitcoin/bitcoin/issues/30294#issuecomment-2191419205)
Ok I see, what you are actually after is an active "eraser", so that you can perform the signing (perhaps on an HWW) with derivation paths included, but then not have them present when passing on to another party. I think this works better for the `Combiner` role, personally.

Your workflow would then work something more like this:

> Create a psbt with bip32derivs included, pass psbt to offline signer which signs inputs.
>
> Pass signed psbt to `combinepsbt` with _some flag_ set to `false
...

💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654615945)
Lock annotations in the cpp file will be ignored by the compiler, because it usually only sees the `.h` file.

💬 brunoerg commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1654619461)
Done.

💬 brunoerg commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1654619596)
Done.

💬 brunoerg commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#issuecomment-2191434797)
Force-pushed:

- I added a commit to change some comments to `self.log.info`
- Addressed: https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653913528 and https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653781334

Thanks, @kevkevinpal and @tdb3.

💬 willcl-ark commented on pull request "Docs improvements":
(https://github.com/bitcoin/bitcoin/pull/30337#issuecomment-2191448472)
Is this even worth keeping open? By my count only 2 of the 8 commits are arguably correct, and it hardly seems worth the review/merge effort at that point...

Checking @nnsW3 [GitHub](https://github.com/nnsW3), it appears that they only provide fly-by spelling changes to a large number of repos, to what ends, who knows?

💬 Sjors commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654633765)
I initially added it only to the header, but that resulted in:

```
node/interfaces.cpp:875:9: warning: calling function 'AssertLockHeldInternal<AnnotatedMixin<std::recursive_mutex>>' requires holding mutex 'cs_main' exclusively [-Wthread-safety-analysis]
AssertLockHeld(cs_main);
^
```


Should I add it in both? Or declare the `override` first (which would need a new header).