💬 maflcko commented on pull request "ci: Remove bash -c from cmake invocation using eval":
(https://github.com/bitcoin/bitcoin/pull/33401#issuecomment-3335311851)
nice, lgtm ACK 8406c1e6321fbc3ce739aa7ccb18c67fb706a4b9

💬 l0rinc commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2379948259)
Not sure about this, I have never seen "inchain transaction" but I have seen "onchain". I would also say "it's not in the set" instead of "on the set" but I *would* say "it's on the blockchain" not "in". What do others think?

💬 l0rinc commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2379949764)
Wouldn't that cause a race condition if the check misses it?

💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379958513)
> Return:
the number of log lines we encountered when matching

This doesn't seem to be the case anymore

💬 hodlinator commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2379991579)
But you've only sent it one block?

💬 hodlinator commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2379988259)
Blocks are "in the chain" to me, like metal links being in a chain. Maybe on-chain tx is more in contrast to off-chain tx, but a tx is in a block, in a chain.

Not important enough that I reacted upon first review, but would definitely suggest changing if you re-touch.

💬 l0rinc commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2380001489)
I think I understood that, but that will be a behavior change, since currently after `Enabling... too recent...` we're still getting a log for `Enabling ... block height above...` while if we only check empty vs non-empty we'd only be logging `Disabled` -> `Enabled` (or vice versa) but not slightly more confusing cases like the one above. Given that the logging was added because even we got confused by this, I thought we should log every state change.

💬 andrewtoth commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2380011100)
I don't understand why you think it's a behavior change. It has nothing to do with logging. Add this on line 2585: `const bool fScriptChecks = (script_check_reason != nullptr);`
Remove changes on lines 2594, 2653.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2380012998)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2380013207)
Fixed

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2380061485)
While the change does seem to fix my personal experience of unexpected script validations, I don't yet understand why reindex does this in the first place - can we fix that instead of adding extra conditions to the script validation checks? Seems a bit hacky and only solves part of the problem, doesn't it?

💬 l0rinc commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2380068845)
> I don't understand why you think it's a behavior change

Compared to the latest state of the code it's a change (not compared `master`).
If we only log when the nullnes changes that will be a different behavior than when the reason changes.
Or am I misunderstanding the comments?

💬 l0rinc commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2380071174)
Right, so what's the advantage of `getblockcount() == 1` compared to "non-zero"?

🚀 glozow merged a pull request: "cli: Handle arguments that can be either JSON or string"
(https://github.com/bitcoin/bitcoin/pull/33230)

💬 glozow commented on pull request "refactor: unify container presence checks (without PR conflicts)":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3335715733)
Apologies, this needs rebase for #33230. I decided to merge it first since it's a functional change and has already gone through a reACK cycle. This one is also very straightforward to rebase.

💬 achow101 commented on pull request "[28.x] More backports":
(https://github.com/bitcoin/bitcoin/pull/33415#issuecomment-3335809587)
ACK a5e4fec4949f61ebbd7d6696f39da26df04515f9

💬 hodlinator commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2380262207)
Makes the int -> bool coercion explicit to the reader, see https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2362266224

🚀 achow101 merged a pull request: "[28.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/33415)

💬 RandyMcMillan commented on pull request "rpcconsole: display signet challenge":
(https://github.com/bitcoin-core/gui/pull/896#discussion_r2380282194)
Testing a proper Qt interface for better formatting now.

💬 RandyMcMillan commented on pull request "rpcconsole: display signet challenge":
(https://github.com/bitcoin-core/gui/pull/896#discussion_r2380297519)
move ChallengeToStdString to src/util/strencodings.cpp for later availability in main window.