💬 darosior commented on pull request "init: drop `-upnp`":
(https://github.com/bitcoin/bitcoin/pull/32500#issuecomment-2884959221)
> if this reintroduces the `settings.json` error, I can also see waiting one more release

An unknown `upnp` entry in `settings.json` would just be ignored? A `upnp` entry in the config file would cause a startup failure. Concept ACK on this basis.

💬 davidgumberg commented on pull request "cmake: Restrict MSVC-specific workaround to affected versions":
(https://github.com/bitcoin/bitcoin/pull/32499#discussion_r2091916322)
Question: Where do these version numbers come from?

👍 darosior approved a pull request: "init: drop `-upnp`"
(https://github.com/bitcoin/bitcoin/pull/32500#pullrequestreview-2844910044)
> An unknown upnp entry in settings.json would just be ignored?

Confirmed.

tACK 301993ebf7f8ec23050e91377e0fd05823bb372a

💬 davidgumberg commented on pull request "cmake: Restrict MSVC-specific workaround to affected versions":
(https://github.com/bitcoin/bitcoin/pull/32499#discussion_r2091919391)
Answer: There is a table on this page https://learn.microsoft.com/en-us/cpp/overview/compiler-versions?view=msvc-170#version-macros

💬 hodlinator commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2885003201)
re-ACK 8f4fed7ec70093e2535423d63e9f9dd400c378ac

Tested that corrected resource path worked when cross-building Linux->Windows:
```
ef128525ed9307b7fefd7b9581e20c8b2c5dd4e4a81b3b97f54ec4c4d26fcbc1 guix-build-8f4fed7ec700/output/dist-archive/bitcoin-8f4fed7ec700.tar.gz
13e21b0584d6cba7a5fd829f654c15862347f12e8ba5eb7a66d9a2e7df53b35e guix-build-8f4fed7ec700/output/x86_64-w64-mingw32/SHA256SUMS.part
d55509ab2ac5b1d4dce7a7b10d4cd6a3a33c7fe63511bb233ea1d8a38cfa44e2 guix-build-8f4fed7ec700/ou
...

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2091927938)
The test coverage is added in the next commit "test: Test for balance update due to untracked output becoming spendable" which I have now moved to be earlier in the PR.

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2091928905)
Done.

This actually revealed a bug in `importdescriptors`, I've added a commit to fix that, although it should actually be moot after the changes in this PR.

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2091931854)
Renamed to `RefreshTXOsFromTx`.

📝 darosior opened a pull request: "policy: make pathological transactions packed with legacy sigops non-standard"
(https://github.com/bitcoin/bitcoin/pull/32521)
The Consensus Cleanup soft fork proposal includes a limit on the number of legacy signature
operations potentially executed when validating a transaction. If this change is to be implemented
here and activated by Bitcoin users in the future, we should make transactions that are not valid
according to the new rules non-standard first because it would otherwise be a trivial DoS to
potentially unupgraded miners after the soft fork activates.

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2091969986)
Done as suggested

💬 achow101 commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2091971018)
As-is is more legible, and this area of code changes significantly in #27865 where having it like it is now makes those changes easier to read.

💬 hodlinator commented on pull request "(RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store":
(https://github.com/bitcoin/bitcoin/pull/32427#issuecomment-2885100223)
### Re: One file per block

FWIW the idea of using one file per block gets me going too. :) It is slightly orthogonal but would be nice to avoid changing formats twice in short succession.

My bikeshed color: Since block hashes start with zeroes, maybe one could shard based off the last two bytes:

Genesis block ends up in something like:
`$DATADIR/blocks/e2/6f/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.dat`
Block [896819](https://mempool.space/block/000000000000000
...

💬 m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2091989087)
Worked, force pushed [449cb9e](https://github.com/bitcoin/bitcoin/commit/449cb9e8985b350a96ddad74a9997ac95118edd0)

💬 davidgumberg commented on pull request "wallet: Keep track of the wallet's own transaction outputs in memory":
(https://github.com/bitcoin/bitcoin/pull/27286#discussion_r2092020703)
Agreed

💬 mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2091928728)
nit: I'd replace "here" with "before setting m_blockfiles_indexed" so the reason becomes more clear.

💬 mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2092011943)
would be good to add a subtest that a run without `-assumevalid` will not connect the invalid block.

💬 mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2091930195)
I'd prefer to call it `activate_all_chainstates` to not introduce a less precise term ("update").

💬 mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2091983914)
This means that in case of a reindex, `ActivateBestChain()`, will be called twice now from `ImportBlocks`, but that shouldn't be a problem because the second call just won't do any work.

💬 mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2092020053)
is there a reason we need to mine that many blocks, and not just a few more to make the test faster?

💬 mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2092014654)
Comment needs updating, `-assumevalid` points to the last block, not the invalid one.