💬 0xB10C commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1895767330)
Yet another alternative would be to set a flag like `CI_FAIL_IF_NO_TCPDUMP_FILE` (or similar) in 00_setup_env_native_asan.sh and add a comment mentioning the dependency:
- if the script name or the container name are renamed, this will still work
- if script is removed, someone might see the comment about the dependency during review
- it's easy to add more tasks to the list of required tasks by just adding `CI_FAIL_IF_NO_TCPDUMP="true"`

💬 sipa commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2559801865)
Concept ACK

💬 sipa commented on pull request "fuzz: Limit wallet_notifications iterations (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31467#discussion_r1895857303)
I don't see too much time difference in a corpus made using both with/without this `FundTx` call, so it seems better to keep the coverage it could provide.

If knapsack is causing slowness, that seems mostly an argument to improve it, reduce its iteration counts, or get rid of it. Paging @murchandamus .

💬 sipa commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#issuecomment-2559886700)
utACK ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e

💬 sipa commented on pull request "descriptor: remove unreachable verification for `pkh`":
(https://github.com/bitcoin/bitcoin/pull/31555#issuecomment-2559898587)
ACK e36640859089baabc46f68217843f96a3ebdc20c

Took me a while to understand why, but it's just because `ParseScript` itself cannot be reached with `ctx == ParseScriptContext::WPKH`. Perhaps an assert or Assume could be added for that general property?

🤔 scgbckbone reviewed a pull request: "wallet: allow lable for external descriptor & disallow label for ranged descriptors"
(https://github.com/bitcoin/bitcoin/pull/31514#pullrequestreview-2520783072)
@achow101 - looks like logic bugs to me

💬 sipa commented on pull request "refactor: std::span compat fixes":
(https://github.com/bitcoin/bitcoin/pull/31540#issuecomment-2559907064)
utACK fa494a1d53f3f030fafe7b533d72b2200428a0fd

💬 sipa commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2559915449)
Concept & approach ACK.

💬 sipa commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2559989402)
utACK 69e95c2b4f99eb4c2af6b2b6cc6a66abfea753df.

I can't imagine any way that this could affect operations later. The key isn't used, so deleting it can't hurt backups, and with or without encryption key no ckey records can be added later. Is that correct?

💬 sipa commented on pull request "coins: warn on shutdown for big UTXO set flushes":
(https://github.com/bitcoin/bitcoin/pull/31534#issuecomment-2559992151)
utACK 5709718b830161b7c2ba0db545ef0cfa98423597

🤔 sipa reviewed a pull request: "test: Add mockable steady clock, tests for PCP and NATPMP implementations"
(https://github.com/bitcoin/bitcoin/pull/31022#pullrequestreview-2520898727)
Neat, utACK 258b2856cdc6e2c054ea573cf57de7021aebc3c5. I did not verify the test scenario byte sequences against the spec.

💬 sipa commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1895944792)
Perhaps document that this represents the expected error.

💬 sipa commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1895945169)
Could be marked `final`.

💬 sipa commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1895945418)
Could be marked `final`.

💬 sipa commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1895947529)
Perhaps document that this must start at nonzero, otherwise `MockableSteadyClock::SetMockTime(m_time)` would disable mocking.

💬 brunoerg commented on pull request "descriptor: remove unreachable verification for `pkh`":
(https://github.com/bitcoin/bitcoin/pull/31555#issuecomment-2560047104)
> Took me a while to understand why, but it's just because ParseScript itself cannot be reached with ctx == ParseScriptContext::WPKH. Perhaps an assert or Assume could be added for that general property?

P2WPKH, right? Sounds good to add an Assume for this, will address it.

👍 danielabrozzoni approved a pull request: "Remove unused variable assignment"
(https://github.com/bitcoin/bitcoin/pull/31497#pullrequestreview-2520971106)
code review ACK b9766c9977e58a9ebc358d9879576376e76a72b1

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2560151983)
`95fc90610a...0ac9caf7be`: do https://github.com/bitcoin/bitcoin/pull/31349#discussion_r1895767330

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2560157622)
`0ac9caf7be...d8cd80e814`: forgot to add a comment in the previous push

👍 danielabrozzoni approved a pull request: "coins: warn on shutdown for big UTXO set flushes"
(https://github.com/bitcoin/bitcoin/pull/31534#pullrequestreview-2521023819)
tACK 5709718b830161b7c2ba0db545ef0cfa98423597

Code looks good to me, I did some manual testing by starting the node with a large dbcache, waiting for the cache to go over 1GiB and then stopping the node, as suggested in the PR description.