💬 polespinasa commented on pull request "refactor: CFeeRate encapsulates FeeFrac internally":
(https://github.com/bitcoin/bitcoin/pull/32750#discussion_r2165819887)
done

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