💬 grzegorzblaszczyk commented on something "":
(https://github.com/bitcoin/bitcoin/commit/c5a9d2ca9e3234db9687c8cbec4b5b93ec161190#commitcomment-148931454)
@ditto-b Nice catch!

💬 luke-jr commented on pull request "cli: Improve error message on multiwallet cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2466952908)
> Removed validation commit which I'll added in a separate PR as https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2354091658 by @jonatack.

Does this exist?

💬 fjahr commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1835806968)
Initially it had the `timeout_factor` and also it seemed nice to have it next to `wait_until` but sure, I have made it a util function now.

💬 fjahr commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#issuecomment-2466975008)
Addressed feedback from @maflcko and made `ensure_for` a util function.

⚠️ Sjors opened an issue: "v27.2 guix build fails with hash mismatch"
(https://github.com/bitcoin/bitcoin/issues/31266)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

```
git checkout v27.2
./contrib/guix/guix-build
...
```

This fails for `gnutls-3.8.1.tar.xz.drv`, so I run that one specifically:

```
$ guix build --cores=1 /gnu/store/b0cvv1d3kz4ilf14iz5b9iq4dafxcv4a-gnutls-3.8.1.tar.xz.drv
The following derivation will be built:
/gnu/store/b0cvv1d3kz4ilf14iz5b9iq4dafxcv4a-gnutls-3.8.1.tar.xz.drv
building /gnu/store/b0cvv1d3kz4ilf14iz5b9i
...

🤔 tdb3 reviewed a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2425867463)
Concept ACK

Great policy update that will provide flexibility to L2s.
Left a few comments, and want to take a deeper look at test cases.

💬 tdb3 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1835811847)
If it's decided to allow non-zero modified fee, then the latter half of the `||` might need to be adjusted here (https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1831104732).

💬 tdb3 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1835815612)
Maybe I'm missing something simple, but could this line insert the parent txid into `processed_parent_set` even when the parent couldn't be checked for dust (`parent_ref` is `nullptr`)? If so, would we want to handle this as an error condition?

Maybe it's not an issue currently (e.g. reliance on 1P1C), but could be defensive to check to protect in the event of future change.

💬 tdb3 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1835805164)
nit: The comment blocks before each function could be updated to use doxygen commands. Could be left for follow-up.

Example:
```diff
/** Must be called for each transaction once transaction fees are known.
* Does context-less checks about a single transaction.
- * Returns false if the fee is non-zero and dust exists, populating state. True otherwise.
+ * @returns false if the fee is non-zero and dust exists, populating state. True otherwise.
*/
bool CheckValidEphemeralTx(const CT
...

💬 tdb3 commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1835796375)
Thought about what guarantees we would have for `get_utxo()` to not return one of the dust. At first glance, looks like currently `get_utxo()` would return the largest utxo (i.e. not dust). Could have a stronger guarantee if we used `get_utxo(confirmed_only=True)` instead (dusty_tx is in the mempool but not yet confirmed). Could be left for a follow-up unless updating for another reason.

💬 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`.