β οΈ Sjors opened an issue: "Include torrent in binary download verification "
(https://github.com/bitcoin/bitcoin/issues/27702)
The binary verification script currently covers bitcoin(core).org but not the torrents. We should add an option to download and verify those as well.
(https://github.com/bitcoin/bitcoin/issues/27702)
The binary verification script currently covers bitcoin(core).org but not the torrents. We should add an option to download and verify those as well.
π¬ MarcoFalke commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554434967)
lgtm ACK 342a0c865d8b4b00a088af7b70b1ee0df1864f5c
This just adds a duplicate entry in the normal case, because the symlink from configure is already followed by python, see https://docs.python.org/3/library/sys.html#sys.path, so the duplicate should be harmless.
Didn't check what Cmake does. I presume it copies?
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554434967)
lgtm ACK 342a0c865d8b4b00a088af7b70b1ee0df1864f5c
This just adds a duplicate entry in the normal case, because the symlink from configure is already followed by python, see https://docs.python.org/3/library/sys.html#sys.path, so the duplicate should be harmless.
Didn't check what Cmake does. I presume it copies?
π¬ sipsorcery commented on pull request "msvc: Provide `ObjectFileName` explicitly":
(https://github.com/bitcoin/bitcoin/pull/27687#issuecomment-1554438083)
ACK b8ed95127b23873db973b2ef4f68d9f04e9c23ae.
Same object file name change has to be done in other places in the msvc build and doesn't affect the final build artifcat.
(https://github.com/bitcoin/bitcoin/pull/27687#issuecomment-1554438083)
ACK b8ed95127b23873db973b2ef4f68d9f04e9c23ae.
Same object file name change has to be done in other places in the msvc build and doesn't affect the final build artifcat.
π fanquake merged a pull request: "msvc: Provide `ObjectFileName` explicitly"
(https://github.com/bitcoin/bitcoin/pull/27687)
(https://github.com/bitcoin/bitcoin/pull/27687)
π¬ hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554443734)
> Didn't check what Cmake does. I presume it copies?
We use [`file(CREATE_LINK ... COPY_ON_ERROR)` ](https://cmake.org/cmake/help/latest/command/file.html#create-link).
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554443734)
> Didn't check what Cmake does. I presume it copies?
We use [`file(CREATE_LINK ... COPY_ON_ERROR)` ](https://cmake.org/cmake/help/latest/command/file.html#create-link).
π fanquake's pull request is ready for review: "25.0 Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27686)
(https://github.com/bitcoin/bitcoin/pull/27686)
π¬ MarcoFalke commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554471723)
Any reason to change it? I presume this would lead to copies instead of hard links when the build dir is on a different storage device? symbolic links could be used to follow to a different storage device.
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554471723)
Any reason to change it? I presume this would lead to copies instead of hard links when the build dir is on a different storage device? symbolic links could be used to follow to a different storage device.
π¬ brunoerg commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1554489167)
> Before I re-ACK, are you gonna squash any commits?
Done!
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1554489167)
> Before I re-ACK, are you gonna squash any commits?
Done!
π¬ sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1554510214)
> Also, I think we dropped support for non-witness compact block relay, so I guess you can drop `txid` from your map and only use `wtxid`?
I believe that there should be some older versions of our software that support witness-compact-block-relay but don't support wtxidrelay, so I think @ajtowns patch is correct to index on both.
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1554510214)
> Also, I think we dropped support for non-witness compact block relay, so I guess you can drop `txid` from your map and only use `wtxid`?
I believe that there should be some older versions of our software that support witness-compact-block-relay but don't support wtxidrelay, so I think @ajtowns patch is correct to index on both.
π¬ hebasto commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554516472)
> Any reason to change it? I presume this would lead to copies instead of hard links when the build dir is on a different storage device?
1. The `SeCreateSymbolicLinkPrivilege` [policy](https://learn.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/create-symbolic-links) is disabled by default for non-administartors on Windows.
2. Even with the `SeCreateSymbolicLinkPrivilege` is enabled, this change is still required on Windows.
> symbolic links could be use
...
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554516472)
> Any reason to change it? I presume this would lead to copies instead of hard links when the build dir is on a different storage device?
1. The `SeCreateSymbolicLinkPrivilege` [policy](https://learn.microsoft.com/en-us/windows/security/threat-protection/security-policy-settings/create-symbolic-links) is disabled by default for non-administartors on Windows.
2. Even with the `SeCreateSymbolicLinkPrivilege` is enabled, this change is still required on Windows.
> symbolic links could be use
...
π€ josibake reviewed a pull request: "25.0 Final Changes"
(https://github.com/bitcoin/bitcoin/pull/27686#pullrequestreview-1434354654)
ACK https://github.com/bitcoin/bitcoin/pull/27686/commits/9ded442dffb827d8762c0becce9d10fa283d1a3a
looks good, although we probably don't need to credit `merge-script` as a contributor
(https://github.com/bitcoin/bitcoin/pull/27686#pullrequestreview-1434354654)
ACK https://github.com/bitcoin/bitcoin/pull/27686/commits/9ded442dffb827d8762c0becce9d10fa283d1a3a
looks good, although we probably don't need to credit `merge-script` as a contributor
π¬ josibake commented on pull request "25.0 Final Changes":
(https://github.com/bitcoin/bitcoin/pull/27686#discussion_r1198915559)
lol credit where credit is due, i guess?
(https://github.com/bitcoin/bitcoin/pull/27686#discussion_r1198915559)
lol credit where credit is due, i guess?
π¬ MarcoFalke commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554519947)
Anyway, should be unrelated to this pull request.
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554519947)
Anyway, should be unrelated to this pull request.
π¬ Sjors commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1554521123)
Ah yes, broken my own #26076.
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1554521123)
Ah yes, broken my own #26076.
π¬ josibake commented on pull request "25.0 Final Changes":
(https://github.com/bitcoin/bitcoin/pull/27686#issuecomment-1554528629)
reACK https://github.com/bitcoin/bitcoin/pull/27686/commits/6675cf809543866792bf6706372fc9ac253d3e99
(https://github.com/bitcoin/bitcoin/pull/27686#issuecomment-1554528629)
reACK https://github.com/bitcoin/bitcoin/pull/27686/commits/6675cf809543866792bf6706372fc9ac253d3e99
π josibake opened a pull request: "doc: filter out merge-script from list of authors"
(https://github.com/bitcoin/bitcoin/pull/27703)
(https://github.com/bitcoin/bitcoin/pull/27703)
π¬ furszy commented on issue "Sign PSBT: Can't verify change output":
(https://github.com/bitcoin-core/gui/issues/732#issuecomment-1554580472)
Small note about this.
This would work if the change address was previously recorded by the wallet (used in a previous tx). If you just obtained it by a `getrawchangeaddress` RPC call, the wallet will currently not detect it as change. Only will detect it as yours (reasons are explained in #25979).
So, would say to present it as "own address" in the dialog instead of checking whether is change or not (external addresses should be presented as yours too).
(https://github.com/bitcoin-core/gui/issues/732#issuecomment-1554580472)
Small note about this.
This would work if the change address was previously recorded by the wallet (used in a previous tx). If you just obtained it by a `getrawchangeaddress` RPC call, the wallet will currently not detect it as change. Only will detect it as yours (reasons are explained in #25979).
So, would say to present it as "own address" in the dialog instead of checking whether is change or not (external addresses should be presented as yours too).
π¬ stickies-v commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554587245)
> Sure. I've updated the PR description with steps to replicate the failure.
Thanks, this helped a lot with my understanding.
I'm not a huge fan of the build system leaking into the test code, if we can avoid it. Someone reading this without knowledge of the build system wouldn't understand why we're appending to `sys.path`, so at the very least I'd suggest adding some documentation and/or a link to this PR.
An alternative approach could be to add a `bitcoin/test/functional/setup.py` fi
...
(https://github.com/bitcoin/bitcoin/pull/27561#issuecomment-1554587245)
> Sure. I've updated the PR description with steps to replicate the failure.
Thanks, this helped a lot with my understanding.
I'm not a huge fan of the build system leaking into the test code, if we can avoid it. Someone reading this without knowledge of the build system wouldn't understand why we're appending to `sys.path`, so at the very least I'd suggest adding some documentation and/or a link to this PR.
An alternative approach could be to add a `bitcoin/test/functional/setup.py` fi
...
π¬ furszy commented on pull request "[WIP] wallet: standardize change output detection process":
(https://github.com/bitcoin/bitcoin/pull/25979#issuecomment-1554595205)
Thanks for the ping marko, rebased.
Yeah, this is still relevant. Change detection is still an issue.
The base work is done, just need some extra love.
Will kept it rebased, so anyone can review/test it up until I'm able to get back to it.
(https://github.com/bitcoin/bitcoin/pull/25979#issuecomment-1554595205)
Thanks for the ping marko, rebased.
Yeah, this is still relevant. Change detection is still an issue.
The base work is done, just need some extra love.
Will kept it rebased, so anyone can review/test it up until I'm able to get back to it.
π furszy converted_to_draft a pull request: "[WIP] wallet: standardize change output detection process"
(https://github.com/bitcoin/bitcoin/pull/25979)
This work aims to define, and implement a base standard mechanism to
detect individual change outputs.
### Context
Currently, the wallet detects whether an output is change or not based
on data stored in the address book.
There is no notion of βchange outputsβ, the wallet detects change scripts.
Connoting that any address book record modification has implications
on all the historical outputs related to that particular destination. Meaning
that all those outputs can either be cha
...
(https://github.com/bitcoin/bitcoin/pull/25979)
This work aims to define, and implement a base standard mechanism to
detect individual change outputs.
### Context
Currently, the wallet detects whether an output is change or not based
on data stored in the address book.
There is no notion of βchange outputsβ, the wallet detects change scripts.
Connoting that any address book record modification has implications
on all the historical outputs related to that particular destination. Meaning
that all those outputs can either be cha
...