💬 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).
(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?
(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.
(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.
(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
...
(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
...
(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
(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!
(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
(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.
(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
(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.
(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.
(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.
💬 TheCharlatan commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049389378)
Mmh, shouldn't that be flipped once the PR for adding it to the release is actually in?
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049389378)
Mmh, shouldn't that be flipped once the PR for adding it to the release is actually in?
🤔 musaHaruna reviewed a pull request: "build: Drop option to disable hardening."
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2776493428)
tested ACK [77e553](https://github.com/bitcoin/bitcoin/commit/77e553ab6a0a98d065884a83490f28eec6df0e23)
Code reviewed and built successfully
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2776493428)
tested ACK [77e553](https://github.com/bitcoin/bitcoin/commit/77e553ab6a0a98d065884a83490f28eec6df0e23)
Code reviewed and built successfully
💬 mzumsande commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2049393357)
Hmm, this is only an issue because `hash_or_height` expects a JSON object, not a string (and a text without quotes is no valid JSON object), so regular string args don't need this. I think the current naming is confusing, maybe renaming the function "`string_to_json_for_cli`" and calling `json.dumps(text)` inside instead of adding quotes (in case the cli is used) would be better to convey why this is done? Or is there a better solution?
(https://github.com/bitcoin/bitcoin/pull/32290#discussion_r2049393357)
Hmm, this is only an issue because `hash_or_height` expects a JSON object, not a string (and a text without quotes is no valid JSON object), so regular string args don't need this. I think the current naming is confusing, maybe renaming the function "`string_to_json_for_cli`" and calling `json.dumps(text)` inside instead of adding quotes (in case the cli is used) would be better to convey why this is done? Or is there a better solution?
💬 fanquake commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049407063)
In that case, adding this seems premature, as this document should represent the state of the release binaries.
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049407063)
In that case, adding this seems premature, as this document should represent the state of the release binaries.
💬 ryanofsky commented on pull request "doc: Add deps install notes for multiprocess":
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049410669)
It looks like this should be changed to "no" to be consistent with the rest of the table since qt, sqlite, etc also have "no". I would think of these all as runtime dependences, but IIUC the distinction this is making is that all of these are statically linked but freetype and fontconfig are dynamically linked.
(https://github.com/bitcoin/bitcoin/pull/32293#discussion_r2049410669)
It looks like this should be changed to "no" to be consistent with the rest of the table since qt, sqlite, etc also have "no". I would think of these all as runtime dependences, but IIUC the distinction this is making is that all of these are statically linked but freetype and fontconfig are dynamically linked.
💬 sipa commented on issue "Wallet should be able to store multiple transactions with same txid":
(https://github.com/bitcoin/bitcoin/issues/11240#issuecomment-2813655649)
Maybe this is clearer:
a. If a transaction with the same txid is in the mempool, then the wallet should store just that one.
b. If no transaction with the same txid is in the mempool, then the wallet should store:
1. The version that was last in the mempool.
2. And, if different from (1), the version that has been confirmed.
If no version of the transaction has been confirmed, b.1 should be rebroadcast.
(https://github.com/bitcoin/bitcoin/issues/11240#issuecomment-2813655649)
Maybe this is clearer:
a. If a transaction with the same txid is in the mempool, then the wallet should store just that one.
b. If no transaction with the same txid is in the mempool, then the wallet should store:
1. The version that was last in the mempool.
2. And, if different from (1), the version that has been confirmed.
If no version of the transaction has been confirmed, b.1 should be rebroadcast.
🤔 glozow reviewed a pull request: "Make TxGraph fuzz tests more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32191#pullrequestreview-2776524353)
utACK 2835216ec09cc2d86b091824376b15601e7c7b8a
(https://github.com/bitcoin/bitcoin/pull/32191#pullrequestreview-2776524353)
utACK 2835216ec09cc2d86b091824376b15601e7c7b8a