💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3129986454)
> It introduces a potential security issue though since we can't validate the output of the separate process. We're just trusting whatever it spits out, and if it were malicious, it could be making fake addresses which results in funds being sent to an attacker., and the user basically wouldn't know.

This is why I raised it in the context of multi-process - multi-process already inherently moves to a world where Bitcoin Core trusts output of other processes, doing this in that context wouldn'
...

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237910584)
Removed in e62bd1b24b6 ci: port tsan-depends-gui

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237906512)
I am actually not 100% sure why this is needed. Warp have it [documented in their docs](https://docs.warpbuild.com/cache/docker_layer_caching#step-1-set-up-docker-buildx-action), but IIRC Cirrus also advised us to set it.

My understanding is that it was necessary for the `gha` caching using custom URL, but perhaps @m3dwards or @fkorotkov could clarify/confirm?

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237908036)
Added a helper job (runtime of ~4 seconds) which has an output whether or not to use cirrus to the subsequent jobs in 8758b6ec333 ci: add job to determine runner type

This reduces the hardcoding to a single env var in the main ci.yml file, which can trivially be patched by forks if desired.

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237910261)
Removed dead code no-longer needed in 3825f903585 ci: port arm 32-bit job (and subsequent job commits)

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237948012)
Set this to run on PRs in fd32aa85b29 ci: port lint by setting `CIRRUS_PR=1` via `if [ ${{ github.event_name }}" = "pull_request" ]; then` which I think should fix this?

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237950268)
Fixed, and now using `$PACKAGES` in 8bb213c64c3 ci: force reinstall of kernel headers in asan

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237909204)
Thanks, I agree.

Removed the input entirely and use `$CONTAINER_NAME` in d6ed832034b ci: add caching actions

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237935705)
Fixed in fd32aa85b29 ci: port lint

💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237982299)
Removed the global makejobs in 89f177406c9 ci: dynamically match makejobs with cores

🤔 jonatack reviewed a pull request: "doc: add note for watch-only wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32866#pullrequestreview-3064816090)
Approach ACK

💬 jonatack commented on pull request "doc: add note for watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32866#discussion_r2238023764)
```suggestion
contains all of the watchonly scripts. `<name>_solvables` contains any scripts that the wallet
```

💬 jonatack commented on pull request "doc: add note for watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32866#discussion_r2238023510)
```suggestion
wallets do not support having both private keys and watch-only scripts, there may be up to two
```

💬 luke-jr commented on pull request "[IBD] log the start of script validation past `assumevalid` block":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238060331)
fScriptChecks isn't a one-way gate. It would be very confusing to see "Started validation" due to a block not an ancestor of the assumevalid block, and then have validation of the real chain continue without validation and no log informing the user it's being skipped again.

💬 HowHsu commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2238103933)
> Would you consider using DepGraph instead of a matrix of indices (it's basically the same thing but more compact)?

Sure, I'll look into it.

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#issuecomment-3130283784)
@sipa, my comments are still relevant as far as I can tell, I think it's a good change, how can I help to advance it?

💬 l0rinc commented on pull request "[IBD] log the start of script validation past `assumevalid` block":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238145186)
Thanks, updated the code to log any such change, let me know what you think!

💬 ajtowns commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2238379741)
`GetIter` is already exposed, it's used in validation.cpp for looking up txs. This would also expose `GetInfo` which converts an iterator into a index-neutral summary of info about the tx.

💬 luke-jr commented on pull request "assumevalid: log every script validation state change":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238396232)
Probably fine/better to log this the first time through for clarity.

💬 l0rinc commented on pull request "assumevalid: log every script validation state change":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2238494034)
I don't mind doing that, but we're already either logging:
> Assuming ancestors of block 00000000000000000001b658dd1120e82e66d2790811f89ede9742ada3ed6d77 have valid signatures.

or
> Validating signatures for all blocks.

Wouldn't it be redundant to always state if we're validating scripts or not and not just when the state changes?