💬 furszy commented on pull request "wallet: init, don't error out when loading legacy wallets":
(https://github.com/bitcoin/bitcoin/pull/32449#issuecomment-2884916792)
> Is it worth it to verify that, previous to this PR, the node with the legacy wallet in its `settings.json` was failing to restart (using `assert_start_raises_init_error`)?

That's what is being tested. You can run the test on master and it will fail due to that behavior.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091873231)
Will do if I need to retouch.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091878196)
Not sure what you would be verified? After invalidateblock and before the crash, we do verify that the coinbase transaction is marked abandoned. This check is to verify that starting after the crash, the wallet has the correct before-invalidate state.

💬 achow101 commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2091878948)
Will do if I need to retouch.

📝 maflcko opened a pull request: "Remove legacy `Parse(U)Int*`"
(https://github.com/bitcoin/bitcoin/pull/32520)
The legacy int parsing is problematic, because it accepts the `+` sign for unsigned integers. In all cases this is either:

* Useless, because the `+` sign was already rejected.
* Erroneous, when third party parsers reject it.
* Confusing, because the `+` sign is neither documented, nor can it be assumed to be present.

Fix all issues by removing the legacy int parsing.

💬 achow101 commented on issue "listunspent, fundrawtransaction, getwalletinfo locks wallet for any other operation":
(https://github.com/bitcoin/bitcoin/issues/27002#issuecomment-2884937158)
> I've suggested before just having a second map of only unspents,

Then you should be interested in reviewing #27865 and its prerequisite #27286

💬 w0xlt commented on pull request "wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`":
(https://github.com/bitcoin/bitcoin/pull/32432#issuecomment-2884949100)
`std::string_view` is known (and usable) at compile time as a view over some static data (it can be constexpr), so it can be concatenated to RPC docs. [1]
`std::string` no, it’s always a runtime, owning, heap‐allocated string, so it won´t work to produce the RPC docs.

[1] https://stackoverflow.com/a/77358740
[2] https://en.cppreference.com/w/cpp/string/basic_string_view

🤔 furszy reviewed a pull request: "wallet: Use `util::Error` throughout `AddWalletDescriptor` instead of returning `nullptr` for some errors"
(https://github.com/bitcoin/bitcoin/pull/32475#pullrequestreview-2844875076)
Code review ACK f17945b347f6a46dee3b56f86a557eaccec1bc72

💬 hodlinator commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628)
https://github.com/bitcoin/bitcoin/blob/7af6e1089ea264e870b26ac83e81e7aa374acbe1/src/bitcoin.cpp#L76-L83

Note that some RPCs take Base64 parameters (finalizepsbt, verifymessage), and Base64 is commonly padded with `=`.

💬 amg1127 commented on issue ""rpcallowip=" configuration directive doesn't accept RFC4193 addresses":
(https://github.com/bitcoin/bitcoin/issues/32433#issuecomment-2884958889)
Thank you for evaluating my bug report.

I had never heard of CJDNS before. On the day I was creating this bug report, I initially thought that the root cause of the issue was an issue with the IPv6 address parser, and got surprised when I found that `bitcoind` accepted a RFC3849 address.

In my opinion, both the error message and the parameter documentation presented by `bitcoind --help` command line should state that CJDNS addresses ([`[fc00::/8]` block](https://github.com/cjdelisle/cjdns/blob
...

💬 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