💬 maflcko commented on issue "build: `test_bitcoin` link failure with `-flto=thin` & address sanitizer":
(https://github.com/bitcoin/bitcoin/issues/33147#issuecomment-3335205925)
Can be closed as fixed?

💬 maflcko commented on pull request "ci: add libcpp hardening flags to macOS fuzz job":
(https://github.com/bitcoin/bitcoin/pull/33462#issuecomment-3335221230)
Concept ACK. The qa-assets repo has a libc++ debug run, so this isn't required, but it seems fast enough to not hurt.

Would be nice to push my suggested diff, so that the xcode is hardcoded again.

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2379892167)
Yes, but that's not an issue.

💬 achow101 commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2379903393)
Yes. The nodes in this test are not always in sync with each other.

🤔 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?