💬 maflcko commented on pull request "refactor: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2757762029)
> I've reorganized the PR as a refactor

I'd say my nack still holds. I don't see the need for adding redundant and questionable benchmarks for refactors. If this was a requirement or even desired for every single refactor of any function, the bench binary would tend to have a number of benchmarks equal to the number of functions in the whole codebase.

🤔 maflcko reviewed a pull request: "docs: improve development guidelines for PR merging"
(https://github.com/bitcoin/bitcoin/pull/32006#pullrequestreview-2721069003)
not sure if it makes sense to extend this further, at the risk of being inaccurate or redundant with other docs.

My preference would be to either remove it or keep it short

💬 maflcko commented on pull request "docs: improve development guidelines for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r2016361836)
this should be self-explanatory, but "first" should be "at the same time"

💬 maflcko commented on pull request "docs: improve development guidelines for PR merging":
(https://github.com/bitcoin/bitcoin/pull/32006#discussion_r2016368520)
fuzz tests aren't limited to security-critical components. Any test framework can be used for any component.

👍 brunoerg approved a pull request: "contrib: document asmap-tool commands more thoroughly"
(https://github.com/bitcoin/bitcoin/pull/32110#pullrequestreview-2721105939)
reACK 704573e016ac40e7cae00a84a9ba532a760e043e

💬 polespinasa commented on pull request "test: remove strict restrictions on rpc_deprecated test":
(https://github.com/bitcoin/bitcoin/pull/32139#discussion_r2016433964)
Oh so you suggest to leave the `self.is_wallet_compiled()` conditional as a skeleton for other future tests?
And leave the log "...if any..." always not commented, so it's still valid with or without tests?
If so, I like the idea will apply.

🤔 janb84 reviewed a pull request: "test: Add encodable PUSHDATA1 examples to feature_taproot"
(https://github.com/bitcoin/bitcoin/pull/32114#pullrequestreview-2721218307)
LGTM ACK [3dd29b1](https://github.com/bitcoin/bitcoin/commit/3dd29b12e1931147145251944d4a1b733be841f9)

- Tested ✅

Given the complexity it is nice to have examples.

💬 l0rinc commented on pull request "refactor: use original log string when no suspicious chars found":
(https://github.com/bitcoin/bitcoin/pull/31923#issuecomment-2757854233)
I've removed the benchmark and update the description, let me know if this seems better.

👍 rkrux approved a pull request: "descriptors: Multipath/PR 22838 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32134#pullrequestreview-2721049447)
crACK 1e1f11e8946126180f4a93d6d99d5981ecfc7cd3

I shared couple naming suggestions though non-blocking.

💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2016352441)
Good catch here, makes sense to reduce the life of this variable here. This `set` is not cleared anywhere in the function that led me to consider whether the older implementation was buggy but it was not because the multipath key specifier can only appear once in the descriptor as per the check on line 1456/1455.

The naming of this variable (not a new change) feels incomplete though. It's a set of derivation indexes/values but this term is not mentioned anywhere in the function. So, somethin
...

💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2016481538)
Again these are derivation indexes/values, so `derivation_values` can be an option instead of just `values`, which feels bland.

💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2016494391)
Using `struct` here seems reasonable to me in order to tie these two related variables together.

💬 rkrux commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#discussion_r2016491048)
> It's a bit more drastic than I was aiming for and still feels incomplete.

Even though I like the suggested change because of its intent, there are trade-offs, and as mentioned it's more than what the author originally intended to do in this pull. So, I will leave its fate to the author.

👋 rkrux's pull request is ready for review: "wallet: remove redundant `Assert` call when block is disconnected"
(https://github.com/bitcoin/bitcoin/pull/32153)

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#discussion_r2016515551)
I really hate copyright discussions, but I think it would good to at least apply it consistently in the project. Definitely not something that should be hashed out on this PR though.

💬 instagibbs commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2016524616)
yep, don't feel strongly on this one but that's probably an expected pattern

💬 instagibbs commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2016525423)
Seems like a matter of taste; leaving as-is for now?

💬 rkrux commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2016541519)
Though a practical suggestion but I think it can be left as-is. It's highly likely that the reader would go through the `run_test()` from where they can navigate to the implementation of `sample_spenders()`.

💬 hodlinator commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2016543766)
Thanks for finding that issue @l0rinc!

I've only observed it on Windows, maybe the socket subsystem or timeouts are different enough there to make it more of an issue.

> However, I don't think anyone has observed this on current master.

Maybe I was running under some less common setup like non-WSL MSVC Debug, or maybe my Windows network stack was having a bad day. (There's an odd chance that I might have been using Linux->Windows cross-built binaries for some reason).

> The first co
...

👍 ryanofsky approved a pull request: "test: avoid treating hash results as integers"
(https://github.com/bitcoin/bitcoin/pull/32050#pullrequestreview-2721408208)
Code review ACK a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc. Nice changes, and sorry about the false bug report