💬 Zero-1729 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1915613633)
It'd be good to clarify here that it is recommended to use `APFS` instead.

```suggestion
- **MacOS**: The exFAT filesystem should not be used, it is recommended to use the APFS filesystem instead. There have been multiple reports of database corruption when using exFAT on MacOS for Bitcoin Core. This appears to be due to filesystem-level issues with exFAT on MacOS. See [Issue #31454](https://github.com/bitcoin/bitcoin/issues/31454) for more details.
```

💬 Zero-1729 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1915615158)
I'd be good to add a recommendation here, too, so the user is aware of possible immediate solutions.

```suggestion
InitWarning(strprintf(_("Specified %s \"%s\" is exFAT which is known to have intermittent corruption problems on MacOS, use APFS instead. "
```

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915615707)
Thanks, but I get a crash immediately:
```
test/fuzz/overflow.cpp:36 TestOverflow: Assertion `check(widen(i) << shift) == CheckedLeftShift(i, j)' failed.
```
Does this correction make sense to you?
```git
diff --git a/src/test/fuzz/overflow.cpp b/src/test/fuzz/overflow.cpp
index defaf9926d..d29273eb6e 100644
--- a/src/test/fuzz/overflow.cpp
+++ b/src/test/fuzz/overflow.cpp
@@ -27,13 +27,13 @@ void TestOverflow(FuzzedDataProvider& fuzzed_data_provider)

const T i = fuzzed_data_p
...

💬 sipa commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2591132620)
@TheBlueMatt I don't follow. A "wait until you have something for me" interface is push-based, and accomplishes all points as far as I can tell.

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915622193)
Thanks, the bottom part of that correction (shifting by `shift` instead of `j`) makes sense but the top part changing `std::numeric_limits<W>::digits ` to `std::numeric_limits<T>::digits` reduces the coverage of the fuzz test by reducing the range of `shift` so I think would not be a good change.

🤔 Zero-1729 reviewed a pull request: "util: detect and warn when using exFAT on MacOS"
(https://github.com/bitcoin/bitcoin/pull/31453#pullrequestreview-2551060227)
Concept ACK

I encountered this error late last year as well. Thank you for working on this. I think having the warning reported is a good measure.

I've dropped a few comments above.

I tested the patch on an M1 MBP running MacOS v15.2 with an exFAT drive (83839a35f07055dd03924b5bdab46bf31df33b35):

```
$ ./build/src/qt/bitcoin-qt --datadir=/Volumes/Little\ Test/

Warning: Specified data directory "/Volumes/Little Test" is exFAT which is known to have intermittent corruption problems
...

💬 luke-jr commented on pull request "wallet: fix rescanning inconsistency":
(https://github.com/bitcoin/bitcoin/pull/31629#issuecomment-2591156186)
Approach NACK.

Seems like we already have logic for this in `max_height`, and if it's not passed, we intend to not stop early... I think adjusting `m_last_processed_block` instead would be the right fix.

💬 luke-jr commented on pull request "leveldb: show non-default options during init":
(https://github.com/bitcoin/bitcoin/pull/31644#issuecomment-2591181620)
How is this useful?

💬 luke-jr commented on pull request "optimization: increase default LevelDB write batch size to 64 MiB":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2591184498)
Can we predict the memory usage spike size? Presumably as we flush, that releases memory, which allows for a larger and larger batch size?

👍 ryanofsky approved a pull request: "test: Add test for rpcwhitelistdefault"
(https://github.com/bitcoin/bitcoin/pull/29858#pullrequestreview-2551114829)
Code review ACK cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b. Left a few more suggestions to consider, but cleanups look good and I think this is pretty easy to understand now.

💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1915663066)
In commit "test: Add test for rpcwhitelistdefault" (cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b)

Note: since test was updated "user3" should now be "strangedude6"

💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1915666383)
In commit "test: Add test for rpcwhitelistdefault" (cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b)

It's a big confusing calling this test rpcwhitelistdefault_1 when nothing in this test actually depends on `rpcwhitelistdefault` value, and the test will pass whether it is 0 or 1. Would suggest renaming the method and calling it with both values to make sure behavior is unaffected. Would suggest:

```diff
--- a/test/functional/rpc_whitelist.py
+++ b/test/functional/rpc_whitelist.py
@@ -94,6 +94
...

💬 ryanofsky commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1915662983)
In commit "test: Add test for rpcwhitelistdefault" (cfce5f5cbacf7d4c209112ac9d35856bed4a3c6b)

Note: since test was updated "user3" should now be "strangedude6"

👍 theuni approved a pull request: "refactor: Avoid UB in SHA3_256::Write"
(https://github.com/bitcoin/bitcoin/pull/31655#pullrequestreview-2551130478)
utACK fabeca3458b38a3d8930cb0cbc866388c3f120f1

💬 mzumsande commented on pull request "wallet: fix rescanning inconsistency":
(https://github.com/bitcoin/bitcoin/pull/31629#issuecomment-2591235952)
> I think adjusting m_last_processed_block instead would be the right fix.

I wanted to do that initially, but I think it's incorrect: If we set `m_last_processed_block` for block 1 until block N within the rescan, and after the rescan has finished we process the outstanding `blockConnected` notification for block 1, `m_last_processed_block` will be set backwards to the the wrong block, and the wallet will be in a inconsistent state until all other outstanding notifications until block N are p
...

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915697166)
Ah, of course. The actual problem is that we are shifting even out of the widened type's range, so it wraps around. Not sure if there is a nice solution to that without re-implementing the logic we are trying to test.

🤔 ismaelsadeeq reviewed a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2551158656)
Concept ACK
I think the added feature here is useful to miners!

The consensus changes involve splitting `CheckProofOfWorkImpl` into two functions:

1. `DeriveTarget`, which returns the proof-of-work target given a compact target.
2. `CheckProofOfWorkImpl`, which performs its normal check to verify whether a block hash satisfies the proof-of-work requirements.

This enables the usage of `DeriveTarget` in other places.

The large diff is due to an alternate mainnet chain data used i
...

💬 ismaelsadeeq commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1915695379)
@Sjors what do you mean by "if found now" here and the other places?

💬 luke-jr commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2591252240)
>All checkpoints and checkpoint logic are removed in a single commit, to make it easy to revert if necessary.

It's easy to revert an entire merge, so feel free to split it up if it makes sense to...

💬 brunoerg commented on pull request "wallet: fix crash during migration due to invalid multisig descriptors":
(https://github.com/bitcoin/bitcoin/pull/31378#discussion_r1915719223)
In 74fa29e12e379d7be8ad2dd261ee68efaf7a9e07: `privkeys` can be removed, it's unused.