💬 mzumsande commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1898015187)
I used `ensure_for` because otherwise the added test might not always have failed on master before (if the first check had been done immediately, before the incorrect notification was processed by the wallet, the balance would have still been 34).

💬 mzumsande commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1898025732)
done

💬 mzumsande commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1898025835)
removed, that was not on purpose

💬 mzumsande commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#discussion_r1898026354)
Makes sense to me! I think that the code is self-explanatory enough that it's not necessary to keep the lines as comments, so I just removed them.

💬 mzumsande commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#issuecomment-2562963913)
[ca07531](https://github.com/bitcoin/bitcoin/commit/ca0753160a8ebc113e08d62c7b6cbe8fa98455e6) to [bc43eca](https://github.com/bitcoin/bitcoin/commit/bc43ecaf6dc0830a27296d3a29428814fed07bb1):
Addressed feedback by @fjahr

📝 brunoerg opened a pull request: "test: descriptor: fix test for `MaxSatisfactionWeight`"
(https://github.com/bitcoin/bitcoin/pull/31570)
To get the maximum size of a satisfaction for a descriptor with no max sig, the parameter `use_max_sig` should be false.

🤔 mzumsande reviewed a pull request: "rpc: Extend scope of validation mutex in generateblock"
(https://github.com/bitcoin/bitcoin/pull/31563#pullrequestreview-2523410930)
utACK fa63b8232f38e78d3c6413fa7d51809f376de75c

I think in theory it would be sufficient to lock `m_chainstate_mutex` for the active chainstate instead of `cs_main` during `createNewBlock`, but I don't think it matters.

👍 murchandamus approved a pull request: "Remove unused variable assignment"
(https://github.com/bitcoin/bitcoin/pull/31497#pullrequestreview-2523421684)
I surmise that I was resetting `should_cut` in the loop that performs the "cut" operation, because performing the cut removes the need for a cut. I agree that the assignment to false is unnecessary, as the variables are recreated at the start of the loop, and starting with fresh variables each loop is easier to parse than instantiating outside of the loop and cleaning up after the operations.

ACK b9766c9977e58a9ebc358d9879576376e76a72b1

💬 pinheadmz commented on issue "b-msghand invoked oom-killer: Master (v28.99) crashing during IBD":
(https://github.com/bitcoin/bitcoin/issues/31561#issuecomment-2563058140)
The node in question finished sync after I ran it inside heaptrace and is humming along just fine now. I printed out the flamegraph although I have trouble interpreting it:


💬 i-am-yuvi commented on issue "test: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=32090)":
(https://github.com/bitcoin/bitcoin/issues/30764#issuecomment-2563080650)
What's the status of this?? @maflcko ??

💬 i-am-yuvi commented on issue "test: 999 of 999 multisig":
(https://github.com/bitcoin/bitcoin/issues/28179#issuecomment-2563083221)
What's the status of this??? @Sjors?? If no one is working I can take this up??

🤔 mzumsande reviewed a pull request: "Add checkblock RPC and checkBlock() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31564#pullrequestreview-2523480948)
The newly introduced `CheckNewBlock()` looks very similar to the existing `TestBlockValidity()` which was just removed from the mining interface in bfc4e029d41ec3052d68f174565802016cb05d41 - it just seems to be a bit more verbose and allows the multiplier for the PoW. Did you consider merging these functions, so that we don't have multiple functions in validation that basically do the same thing?

👍 tdb3 approved a pull request: "test: descriptor: fix test for `MaxSatisfactionWeight`"
(https://github.com/bitcoin/bitcoin/pull/31570#pullrequestreview-2523544300)
ACK 00ec80b24ec32ac695d947587dd0f860fc6f1efa
lgtm

🤔 tdb3 reviewed a pull request: "Add checkblock RPC and checkBlock() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31564#pullrequestreview-2523532175)
Concept ACK

> Instead of a multiplier I could also allow a custom target. This would give weak block systems more flexibility on how to derive their difficulty.

I'd be in favor of this. It's more flexible, could prevent interface churn, and if the user wishes to create the custom target from a simple multiplication they could do the multiplication on their end.

💬 tdb3 commented on pull request "Add checkblock RPC and checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31564#discussion_r1898141894)
`CheckWeakProofOfWork()` seems really similar to `CheckProofOfWorkImpl()`. Maybe I'm missing something simple. What was the rationale for creating the new function vs adding an argument?

✅ pilab-gwon closed an issue: "[Testnet] Insufficient data or no feerate found"
(https://github.com/bitcoin/bitcoin/issues/31032)

💬 fjahr commented on pull request "validation: Send correct notification during snapshot completion":
(https://github.com/bitcoin/bitcoin/pull/31556#issuecomment-2563545464)
re-ACK bc43ecaf6dc0830a27296d3a29428814fed07bb1

⚠️ SwimmingRieux opened an issue: "failure in wallet_sendall.py --descriptors"
(https://github.com/bitcoin/bitcoin/issues/31571)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

When I try to run functional tests using `build/test/functional/test_runner.py`, only 1 test gets failed:


error :
`Temporary test directory at /tmp/test_runner_₿_🏃_20241227_142837
usage: create_cache.py [options]
create_cache.py: error: unrecognized arguments:
...

💬 jaeheonshim commented on issue "Stale code (that has no effect)":
(https://github.com/bitcoin/bitcoin/issues/31558#issuecomment-2563689342)
With the current unit tests, I don't think ceil away from zero will pass unit tests. The way the logic is currently implemented, negative values are rounded up towards zero unless they are greater than -1, in which case they are instead rounded down towards -1. I'm not sure if this is the intended behavior, but if a fix like this were to be implemented:

```
double value = nSatoshisPerK * nSize / 1000.0;
nFee = static_cast<CAmount>(std::copysign(std::ceil(std::abs(value)), value));
```

T
...

📝 jaeheonshim opened a pull request: "refactor: Remove redundant edge case in fee rate rounding logic"
(https://github.com/bitcoin/bitcoin/pull/31572)
Positive fractional fee rates are already rounded away from zero, so the first half of this if-statement is unnecessary:

```
if (nFee == 0 && nSize != 0) {
if (nSatoshisPerK > 0) nFee = CAmount(1);
if (nSatoshisPerK < 0) nFee = CAmount(-1);
}
```

Addresses #31558. This fix improves code readability.

🚀 ryanofsky merged a pull request: "net, net_processing: additional and consistent disconnect logging"
(https://github.com/bitcoin/bitcoin/pull/28521)