🤔 l0rinc reviewed a pull request: "qa: Improvements to debug_assert_log + busy_wait_for_debug_log"
(https://github.com/bitcoin/bitcoin/pull/33423#pullrequestreview-3268886010)
The error message seems off to me now, we're potentially logging lines as missing that we haven't checked.

💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379915262)
Seems this was always eagerly evaluated since https://github.com/bitcoin/bitcoin/commit/fa3e9f7627784ee00980590e5bf044a0e1249999#diff-7f22a082e3a80ca2f212e36193f87dd80237035afedb7f15b116ef7fa265f3eeR245-R248 - the current approach of only calculating it once when the loop also terminates makes sense

nit: I know it's a long line but I would still prefer this merged
```suggestion
self._raise_assertion_error(f'Unexpected message "{unexpected_msg}" found in log:\n\n{join_log(log
...

💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379926157)
that's not necessarily true, we have just stopped searching after the first failure, right?

💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379920746)
this basically pops until the last element is still in logs, stops otherwise, right?
```suggestion
while remaining_expected and remaining_expected[-1] in log:
remaining_expected.pop()
```

💬 l0rinc commented on pull request "qa: Improvements to debug_assert_log + busy_wait_for_debug_log":
(https://github.com/bitcoin/bitcoin/pull/33423#discussion_r2379922589)
it would be slightly more pythonic to do

```suggestion
if not remaining_expected:
```

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