💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#issuecomment-3003478781)
db63d5bf81895d78d39fe8cf94ded6f63d81d614 removes an unused include and some nit on arg names.
990010f49ca97479f9fe46e288ed0f0fbb0fde94 modifies the code to use uint instead of ints see https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2165236159 for context

💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2165877940)
That would make sense. Implemented on a separated commit 990010f49ca97479f9fe46e288ed0f0fbb0fde94 (will clean and squash later).

@sipa, I would like to know your opinion on this. Is there any reason you used int32 for `EvaluateFee` in the first place? The two `Assume` at the beginning of the function cut the function to only positive numbers.

🤔 ismaelsadeeq reviewed a pull request: "util: detect and warn when using exFAT on MacOS"
(https://github.com/bitcoin/bitcoin/pull/31453#pullrequestreview-2956792851)
Code review and tested ACK 2ad18fc9784e6a37e453bf844ba10c4e46f54754

bitcoind
```
2025-06-25T06:12:15Z [warning] The following paths are on exFAT which is known to have intermittent corruption problems on MacOS: data directory ("/Volumes/StoreJet/bitcoin_temp"). Move these directories to a non-exFAT formatted drive to avoid corruption.
```

gui
<img width="577" alt="Screenshot 2025-06-25 at 07 29 37" src="https://github.com/user-attachments/assets/1e1340e6-9e3e-4aed-beb6-8ab5a5eed678" /
...

💬 ismaelsadeeq commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2165887565)
nitty-nit: right now the ouput seems to be plural, i.e directories when it's one so this might be better?
```suggestion
if (!exfat_paths.empty()) {Add commentMore actions
for (const auto& path : exfat_paths) {
InitWarning(strprintf(_("The specified %s is on exFAT which is known to have intermittent corruption problems on macOS. "
"Restart with non-exFAT formatted drive to avoid corruption."),
path))
...

💬 ismaelsadeeq commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2165892974)
e.g
```
2025-06-25T06:13:16Z [warning] The following paths are on exFAT which is known to have intermittent corruption problems on MacOS: blocks directory ("/Volumes/StoreJet/blocks_temp/blocks"). Move these directories to a non-exFAT formatted drive to avoid corruption.
Warning: The following paths are on exFAT which is known to have intermittent corruption problems on MacOS: blocks directory ("/Volumes/StoreJet/blocks_temp/blocks"). Move these directories to a non-exFAT formatted drive to a
...

💬 willcl-ark commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2165924249)
I'm pretty sure this is actually just two different warning styles both being logged to the same place by the user. I haven't tested, but this code: https://github.com/bitcoin/bitcoin/blob/ad654a4807cd584be9ffcd8640f628ab40cb5170/src/noui.cpp#L33-L46 logs one error to log (with timestamp) and when running the daemon the same error (without timestamp) to `stderr`.

When running daemon in the foreground, both messages appear printed in the console. But IMO this makes sense for the daemonised pro
...

💬 ismaelsadeeq commented on pull request "test: fix an incorrect `feature_fee_estimation.py` subtest":
(https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-3003557599)
cc @glozow Is this rfm? I'd like to make a quick follow-up on this, as mentioned in
https://github.com/bitcoin/bitcoin/pull/32463#issuecomment-2930709631

Also, this can be backported to 29.1 cc @fanquake.

💬 vasild commented on pull request "test: refactor out same-txid-diff-wtxid tx to reuse in other tests":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2165925720)
Here is an incomplete wip that [tries to do that](https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073927377):

<details>
<summary>[patch] malleate an existent transaction by flipping s to ORDER - s</summary>

```diff
diff --git i/test/functional/test_framework/key.py w/test/functional/test_framework/key.py
index 682c2de35f..af62dc353a 100644
--- i/test/functional/test_framework/key.py
+++ w/test/functional/test_framework/key.py
@@ -26,12 +26,49 @@ def TaggedHash(tag, data):
...

💬 ismaelsadeeq commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r2165927394)
Makes sense, thanks for explaining.

👍 hodlinator approved a pull request: "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)"
(https://github.com/bitcoin/bitcoin/pull/32564#pullrequestreview-2956870188)
ACK b1f7877bb79f72d212d0ad6907ec1057f96693a7

Cleans up workarounds made for older MSVC versions.

Mainly code review, but also tried applying the revert on top of PR base myself and found some nits.

💬 hodlinator commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#discussion_r2165938792)
nits in bc4fb79a4e2ce90f1340328cae033e008227c166:
When I applied a revert of 63317103c9f2b0635558da814567bb79c17ae851 to the PR base (ff1ee102c4babadd85e4a18234061f10daaa119d), and noticed it brought back the space before the literal name:
```C++
friend constexpr Type operator"" _mst(const char* c, size_t l);
```
The space was removed in faf21625652fd0d4bbf9b86fd9ebedb5857505ea, so I guess you manually edited the revert to account for that? Might want to mention it in the commit message.

...

💬 hodlinator commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#discussion_r2165971583)
nit: Tangential, but `consteval`/`constexpr` implies `inline` AFAIK. Understand if you don't want to diverge more from the commits you revert/apply in this PR though.

💬 vasild commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2165986820)
Consider this:

```diff
--- i/test/functional/wallet_keypool.py
+++ w/test/functional/wallet_keypool.py
@@ -124,16 +124,13 @@ class KeyPoolTest(BitcoinTestFramework):

# refill keypool with three new addresses
nodes[0].walletpassphrase('test', 1)
nodes[0].keypoolrefill(3)

# test walletpassphrase timeout
- # CScheduler relies on condition_variable::wait_until() which is slightly
- # less accurate than libevent's event trigge
...

👍 vasild approved a pull request: "rpc: use CScheduler for HTTPRPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2956960446)
ACK e70c2087b00f6cfe1a95e9d82e8cb7e01b7f7675

Would be nice to take https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2165986820

🤔 janb84 reviewed a pull request: "rpc: Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-2956925945)
A few comments / NITs on the documentation side of this PR.

💬 janb84 commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2165976679)
Shouldn't this file also be referenced in the README.md located in this directory ?
```
This documentation is not an exhaustive list of all policy rules.

- [Mempool Limits](mempool-limits.md)
- [Mempool Replacements](mempool-replacements.md)
- [Packages](packages.md)
```

💬 janb84 commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2165967954)
This feels to me more as a PR comment than something that belongs in this documentation.

👍 l0rinc approved a pull request: "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/31713#pullrequestreview-2956921144)
Code review ACK f22f4f75e252e4f2171af34e4f92f3e6a396f227

Someone else with more experience should also review it, I have only lightly tested it locally.

💬 l0rinc commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2165968443)
```suggestion
for (uint32_t i{0}; i < 1'000'000; ++i) {
```
(locally `100'000` is enough for me to reproduce the failure with incorrect destructor and clone - this test may be too heavy otherwise)

💬 l0rinc commented on pull request "miniscript refactor: Remove unique_ptr-indirection (#30866 follow-up)":
(https://github.com/bitcoin/bitcoin/pull/31713#discussion_r2165966175)
+1 for the example in the commit message (if you touch again consider reducing the indentation)
we could even lightly validate the clone here by comparing the two depths for good measure