💬 tdb3 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1835810249)
The function also returns `nullopt` for an invalid package. Maybe I'm missing something, but this seems to conjoin a failure case (invalid package) with the success case (all dust properly spent). Something like https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1834726168 could increase consistency.

👍 tdb3 approved a pull request: "test: Introduce ensure_for helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2425909929)
code review re ACK dde84e2b2b6a64f5f67d4dff6e3fb2fc7d2c5a77

Changes are focused on the movement of `ensure_for()` to util (with appropriate adjustments to imports and calls to it).

💬 pablomartin4btc commented on pull request "cli: Improve error message on multiwallet cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2467240554)
> > Removed validation commit which I'll added in a separate PR as [#26990 (comment)](https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2354091658) by @jonatack.
>
> Does this exist?

So, this was the 2nd commit https://github.com/bitcoin/bitcoin/commit/3d63fc976d616436d64335b15a918ffba1883b9a that I've removed for simplicity of this PR plus one of those validations: "only 1 cli-command can run at a time (eg can't run -generate and
-getinfo at the same time)," was done nicely on
...

👍 rkrux approved a pull request: "doc: Fixup bitcoin-wallet manpage chain selection args"
(https://github.com/bitcoin/bitcoin/pull/31264#pullrequestreview-2426166980)
crACK fa729ab4a276c3462e390bf2fab6cad93d3a590d

💬 rkrux commented on pull request "doc: Fixup bitcoin-wallet manpage chain selection args":
(https://github.com/bitcoin/bitcoin/pull/31264#discussion_r1835988609)
I had noticed `-2022` in few others files as well, wonder why they don't say `-present`. Good to see these getting fixed one by one.

💬 rkrux commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1836020116)
Since [this commit](https://github.com/bitcoin/bitcoin/commit/cc21876b125930f8320dbb95016f9ee7c1ffec55) was checking for the presence and call-ability of the passed methods already, the main intent of [this comment](https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1827274569) was to add on to it by allowing the execution of all the methods in case of failure of any one of them.

> test-only dev-only debug-only feature

I believe keeping this^ in mind, it makes sense to just throw th
...

💬 rkrux commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1836023847)
With the above approach, I see the following outputs that seem okay.

Error when a non existing attribute is passed.
```
2024-11-11T06:12:11.578000Z TestFramework (INFO): Attempting to execute method: test_args_l
2024-11-11T06:12:11.579000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "/Users/rkrux/projects/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
self.run_custom_tests()
~~~~~~~~~~
...

💬 maflcko commented on issue "v27.2 guix build fails with hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/31266#issuecomment-2467397516)
> sha256 hash mismatch for /gnu/store/rfq2b86kacgk0aslndpawk8gq912n9xj-gnutls-3.8.1.tar.xz:
> expected hash: 1742jiigwsfhx7nj5rz7dwqr8d46npsph6b68j7siar0mqarx2xs
> actual hash: 1jp7wmciqz9cmxvcqfn8lf2c0p8w6xp9xjrvk1z9lq0faswk2102

I'd say this is an upstream issue (of guix and gnutls). It would be good to notify either, or both. Also, it would be good to preserve the file, and compare it with the previous one.

🤔 rkrux reviewed a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2426297404)
Concept ACK 1d722660a65522539872c09ae8a3ba8c9ca55b77
The util function looks quite different from the last time I reviewed, simpler and more readable now!

Couple points:
1. The PR needs a rebase so that it works with the new build system, unable to build and test it in local atm.
2. I agree with @vasild's [comment](https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2415263506) that the first commit should not be importing the helper function only to be used in the next commit,
...

💬 rkrux commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1836069357)
Nit: The 3rd argument can be called `error_message`.

💬 maflcko commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1836124767)
nit in b2ac698ce102edaf8039f42b8793ca10deaa0a53: Can use f-strings?

💬 maflcko commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1836135115)
8362ec6ebbe2410c551c4d6550438426c425ce3c: I am not sure this is accurate. The time can now be scaled, but in reality it is really a fixed time.

🤔 maflcko reviewed a pull request: "test: Introduce ensure_for helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2426388517)
almost ack dde84e2b2b6a64f5f67d4dff6e3fb2fc7d2c5a77 🚿

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: almost ack dde84e2b2b6a
...

💬 maflcko commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1836124512)
nit in b2ac698ce102edaf8039f42b8793ca10deaa0a53: Can remove unused kwargs?

💬 hodlinator commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1836226771)
Time to return to C89? ;)
```suggestion
while (strchr("#0- +", *it)) ++it;
```

📝 maflcko opened a pull request: "refactor: Drop deprecated space in operator""_mst"
(https://github.com/bitcoin/bitcoin/pull/31267)
The space is deprecated according to https://en.cppreference.com/w/cpp/language/user_literal#Literal_operators and compilers will start to warn about this. For example, GCC-15 should warn when compiling under C++23, according to https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html#index-Wdeprecated-literal-operator and Clang-20 will do so by default, according to https://clang.llvm.org/docs/DiagnosticsReference.html#wdeprecated-literal-operator.

Fix it by removing the unused an
...

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1836348991)
I don't think it is right to use mockable time here. Also, `GetTime` is deprecated, so what about `TicksSinceEpoch<std::chrono::nanoseconds>(SystemClock::now())`?

Also, I think it would be better to switch `test_name / rand_str` to `rand_str / test_name`, so that all unit tests from the same time are bundled together. This would also be identical to the functional test runner.

💬 maflcko commented on pull request "doc: Fix missing comma in JSON example in REST-interface.md":
(https://github.com/bitcoin/bitcoin/pull/31259#issuecomment-2467792806)
lgtm ACK 5e3b444022c354ad4d69e3142f7bc98d723c9e29

💬 TheCharlatan commented on pull request "refactor: Drop deprecated space in operator""_mst":
(https://github.com/bitcoin/bitcoin/pull/31267#issuecomment-2467799611)
```
git grep -i 'operator"" '
src/test/fuzz/miniscript.cpp:using miniscript::operator"" _mst;
src/test/miniscript_tests.cpp:using miniscript::operator"" _mst;
```
Might want to correct those two imports as well?

💬 maflcko commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1836375257)
Looks like this coverage should be preserved?

See also https://corecheck.dev/bitcoin/bitcoin/pulls/31248