💬 ismaelsadeeq commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1828502796)
Done, thanks

💬 ismaelsadeeq commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1828505297)
Some tests might not have the test prefix, though. I don’t think we can use the `test_` prefix to catch all sub-tests; rather, the goal is just to enable running individual methods that can be sub-tests.

👍 ryanofsky approved a pull request: "test: Prove+document ConstevalFormatString/tinyformat parity"
(https://github.com/bitcoin/bitcoin/pull/30933#pullrequestreview-2414300187)
Code review ACK a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b. I didn't dig into previous discussion and it seems possible another approach to extending the tests might have advantages over this one. But the code change looks good and seems like an easy way of leveraging existing test cases to check for compatibility with tinyformat.

💬 ryanofsky commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1828504699)
In commit "test: Prove+document ConstevalFormatString/tinyformat parity" (a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b)

The name `TfmF` doesn't have any clear meaning to me, and usage of the function is also complicated by the need to use tuple_cat everywhere. I think the code would be clearer if the function were less generic and did not require passing a tuple. Maybe would suggest:

<details><summary>diff</summary>
<p>

```diff
--- a/src/test/util_string_tests.cpp
+++ b/src/test/util_str
...

💬 ryanofsky commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1828510180)
In commit "test: Prove+document ConstevalFormatString/tinyformat parity" (a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b)

Maybe drop WrongNumArgs and just pass CorrectArgs? Unless I'm missing something it seems like PassFmtIncorrect could choose the wrong number of args arbitrarily like PassFmt does.

💬 achow101 commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2455881092)
As this is basically a complete rewrite of the base58 encoding and decoding code, I don't think the review effort required to review this is worth it relative to the unimportance of these functions.

💬 faisalazuredev commented on pull request "Update secp256k1 subtree to v0.6.0":
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2455983357)
merge it

👍 theStack approved a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2414414598)
ACK 131bed19bdfc922328cad9781fa9122b6810a8fd

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1828578790)
nit: should ideally be already part of the commit that introduces this function for minimal diff (here and in header)

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1828577696)
nit: missing closing double quote

📝 secp512k2 opened a pull request: "doc: Fix word order in developer-notes.md"
(https://github.com/bitcoin/bitcoin/pull/31220)
This pull request fixes a word order error in developer-notes.md.

Before:

"In cases where do you call .c_str(), you might want to additionally check that the string does not contain embedded '\0' characters..."

After:

"In cases where you do call .c_str(), you might want to additionally check that the string does not contain embedded '\0' characters..."

Explanation:

The sentence had incorrect word order, making it grammatically incorrect. Rearranging "do you" to "you do" correct
...

👍 rkrux approved a pull request: "test: enable running independent functional test sub-tests"
(https://github.com/bitcoin/bitcoin/pull/30991#pullrequestreview-2414999263)
ACK e96823bdcc2317fbf0bed64e53a1b3e5bb8f00f6

Thanks for quickly addressing the feedback.

💬 rkrux commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1828979986)
> or other methods.

Nit: This can be reworded though because the following snippet works.

```
def calculate(self, val):
return val

def sample_calculator(self):
self.log.info("Sample calculator running")
x = self.calculate(2 + 3)
self.log.info(f'Calculated x to be: {x}')
```

```
➜ bitcoin git:(09-2024-run-ind-functional-test) ✗ build/test/functional/feature_config_args.py --test_methods test_log_buffer test_args_log sample_calculator
``
...

💬 rkrux commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1828966839)
Ok I didn't know that all tests will not have the test prefix. Sounds good then.

💬 remyers commented on pull request "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees":
(https://github.com/bitcoin/bitcoin/pull/30080#issuecomment-2456675338)
> I think the obvious way to implement this feature is to just detect when the fee paid is greater than the fee needed, and send the excess to the recipient specified, just as we already do when there are change outputs.

Thanks for the feedback. I agree and think for our use we can even do that diff outside of bitcoind. I'll update the PR.

💬 willcl-ark commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2456696504)
> I can test this out on a Ryzen CPU with ZFS on SSD (but with only dual channel DDR4).
>
> * Is there anything else about the config that you can provide so I can try to reproduce?
>
> * What is your ZFS pool setup like?
>
> * Is your SSD also SATA-connected like OP?
>
> * Are you running any Bitcoin Core options that may be relevant?

OK I could not reproduce this on either a native ZFS zpool nor an "ext4 on ZFS" zpool (configurations found here: https://github.co
...

💬 vasild commented on issue "Setting torcontrol overrides proxy address":
(https://github.com/bitcoin/bitcoin/issues/25265#issuecomment-2456720675)
> According to my understanding bitcoin core designed automatically to configure tor when using `torcontrol,`

It automatically configures the location of the Tor proxy, used for outgoing connections to Tor. Don't confuse this with the creation of the Tor hidden service for accepting incoming connections from Tor, that is controlled by `-listenonion=0|1`.

@kroese, it is a bit unclear from the OP, did it connect to torcontrol and retrieve an invalid proxy from there? That is, `127.0.0.1:9050
...

💬 kroese commented on issue "Setting torcontrol overrides proxy address":
(https://github.com/bitcoin/bitcoin/issues/25265#issuecomment-2456762045)
@vasild I cannot remember anymore, it was two years ago that I ran into this behaviour. So feel free to close it.

💬 l0rinc commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2456773318)
Thanks for checking it out @achow101.
I could make the diff minimal, but given that it's not that important, I will leave it as is - until someone starts complaining that `listunspent` is slow :)

👍 vasild approved a pull request: "test: create assert_not_equal util"
(https://github.com/bitcoin/bitcoin/pull/29500#pullrequestreview-2415263506)
Almost ACK 1d722660a6, modulo squash of the two commits.

This is a nice addition.

It is better to squash the two commits into one because IMO it does not make sense to first add the import in one commit and in another commit to use it. That way it becomes difficult to review whether an unnecessary import was added to some file. Also some python linter that tests each commit could be upset about the unused imports in the first commit.

I found it easier to read the diff itself instead of
...