💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049251829)
Done.

💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049252754)
Done.

💬 ryanofsky commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813429363)
Code review ff4ea070aa4b1ed1ebb6dcf62e01c9dd57319f69

Thanks for adding this. Somehow I thought the capnproto dependencies were already listed, but that change is sitting in a commit in #10102 (bbcbe85cef21049b5f321265264e2c4fe394d209).

Few suggestions:

- Would suggest changing "Multiprocess Dependencies" to "IPC Dependencies" since the only thing they are use for on master is -ipcbind feature, and multiprocess functionality is a superset of IPC functionality.
- Might be good to state e
...

💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049254166)
I'll leave this as is for now. A bit out of scope for this PR.

💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049254712)
Agreed, done.

💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049256701)
Done. I love structured bindings now.

💬 marcofleon commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#discussion_r2049256933)
Done.

💬 furszy commented on pull request "wallet: Automatically repair corrupted metadata with doubled derivation path":
(https://github.com/bitcoin/bitcoin/pull/29124#discussion_r2049089569)
it would be better to use `find()` instead of `at()` in case the key metadata is not in the wallet. E.g. an edge case.. could probably happen when `hdKeypath` path matches the 0.21 bug pattern but fails the hardened bits checks (this requires user's manual intervention to occur).

💬 l0rinc commented on pull request "ci: switch to LLVM 20 in tidy job":
(https://github.com/bitcoin/bitcoin/pull/32226#issuecomment-2813461635)
Was `modernize-type-traits` removed from `src/.clang-tidy` in latest push on purpose?

💬 fanquake commented on pull request "ci: switch to LLVM 20 in tidy job":
(https://github.com/bitcoin/bitcoin/pull/32226#issuecomment-2813463406)
Yes.

💬 hebasto commented on issue "ci: failure in Windows cross-test":
(https://github.com/bitcoin/bitcoin/issues/32291#issuecomment-2813495750)
Reminds https://github.com/bitcoin/bitcoin/issues/27352.

💬 l0rinc commented on pull request "ci: drop -priority-level from bench in win cross CI":
(https://github.com/bitcoin/bitcoin/pull/32288#issuecomment-2813509164)
Seems related: https://github.com/l0rinc/bitcoin/actions/runs/14519683955/job/40737543737?pr=8
> Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
what(): filesystem error: cannot remove all: The process cannot access the file because it is being used by another process [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\af1d7c4
...

💬 TheCharlatan commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813519306)
> Thanks for adding this. Somehow I thought the capnproto dependencies were already listed, but that change is sitting in a commit in https://github.com/bitcoin/bitcoin/pull/10102 (https://github.com/bitcoin/bitcoin/commit/bbcbe85cef21049b5f321265264e2c4fe394d209).

Heh, I was looking around if either Sjors or you already did this in one of the open PRs after forgetting to install capnproto on a new machine, but forgot the check the original PR...

Updated ff4ea070aa4b1ed1ebb6dcf62e01c9dd573
...

💬 Sjors commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#issuecomment-2813533741)
re-ACK 5ae18914805f0a78dde85cfe2a7876c4e52c0fe7

👍 ryanofsky approved a pull request: "doc: Add deps install notes for multiprocess"
(https://github.com/bitcoin/bitcoin/pull/32293#pullrequestreview-2776404590)
Code review ACK 5ae18914805f0a78dde85cfe2a7876c4e52c0fe7. Thanks for the change and updates!

💬 ryanofsky commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049339526)
In commit "doc: Add deps install notes for multiprocess" (5ae18914805f0a78dde85cfe2a7876c4e52c0fe7)

May want to update version used to 1.1.0 to match depends https://github.com/bitcoin/bitcoin/blob/bfeacc18b36132ba8ac70142133cd6c0e63b6763/depends/packages/native_capnp.mk#L2

💬 fanquake commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049347883)
Shouldn't this be "no" for runtime? If this goes into a release, it won't be a runtime dep.

💬 glozow commented on issue "Test Package Accept":
(https://github.com/bitcoin/bitcoin/issues/32160#issuecomment-2813573678)
I was referring to testmempoolaccept:

>> Given that we want testmempoolaccept to relax the requirement that the transaction should be topologically connected

> There is already no requirement of connectedness now

💬 l0rinc commented on pull request "refactor: reenable `implicit-integer-sign-change` check for `serialize.h`":
(https://github.com/bitcoin/bitcoin/pull/32296#issuecomment-2813590511)
Since I can't often reproduce the exact CI behavior locally, I have verified it by pushing to my own remote instead - which failed as expected when I have removed something that still needed the suppression: https://github.com/l0rinc/bitcoin/pull/8

I've simplified the commit structure, should be fine now - except for the benchmark failures on master which will likely fail.

💬 glozow commented on issue "Wallet should be able to store multiple transactions with same txid":
(https://github.com/bitcoin/bitcoin/issues/11240#issuecomment-2813596861)
> I think this really calls for storing up to 2 variants of each transactions: the one we expect to confirm, and optionally the one we've seen accepted in a block.

Here, does "expect to confirm" mean the "best" or smallest size / highest feerate variant? So if "the one we've seen accepted in a block" is reorged and rejected from mempool for being nonstandard, we wouldn't try to rebroadcast that, we'd rebroadcast the "best" one.