💬 ryanofsky commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2851586658)
Concept ACK.
I think I agree with @purpleKarrot that it is bad to override CMAKE_ variables, and in general the build does too much overriding (which I've mentioned before in places like https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329412411).
But it seems like the PR is moving in the right direction and making a strict improvement by overriding less aggressively than before. I think hebasto's point about wanting CMAKE_BUILD_TYPE=Release (and release builds in multi-config
...
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2851586658)
Concept ACK.
I think I agree with @purpleKarrot that it is bad to override CMAKE_ variables, and in general the build does too much overriding (which I've mentioned before in places like https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2329412411).
But it seems like the PR is moving in the right direction and making a strict improvement by overriding less aggressively than before. I think hebasto's point about wanting CMAKE_BUILD_TYPE=Release (and release builds in multi-config
...
💬 fjahr commented on pull request "[EXPERIMENTAL] Schnorr batch verification for blocks":
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2851688561)
> While testing this, I observed a decline in performance as the number of threads increased.
Can you share the setup for these benchmarks so I can test them? Thanks!
(https://github.com/bitcoin/bitcoin/pull/29491#issuecomment-2851688561)
> While testing this, I observed a decline in performance as the number of threads increased.
Can you share the setup for these benchmarks so I can test them? Thanks!
👍 ryanofsky approved a pull request: "wallet: Ensure best block matches wallet scan state"
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2815487111)
Code review ACK de7b5eda7ef04e875850fe594de2a89992b7bd46. No significant changes since the last review other than a rebase and a CI fix.
Left some suggestions that I think could make the PR a little easier to understand and review, but they are not important so feel free to ignore.
(https://github.com/bitcoin/bitcoin/pull/30221#pullrequestreview-2815487111)
Code review ACK de7b5eda7ef04e875850fe594de2a89992b7bd46. No significant changes since the last review other than a rebase and a CI fix.
Left some suggestions that I think could make the PR a little easier to understand and review, but they are not important so feel free to ignore.
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2073864891)
In commit "wallet: Remove chainStateFlushed" (2d72a8894cfb609ee616b098e165c722183ba325)
Would it be easily possible to squash this commit and the earlier commit "wallet: Remove WriteBestBlock from chainStateFlushed" (35b636469983fdcd5f8e62448f4733effe596e41) together?
I feel like that wouild make the PR easier to understand because the earlier commit doesn't provide any rationale while the commit message here is a little misleading because the commit at this point is only removing dead cod
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2073864891)
In commit "wallet: Remove chainStateFlushed" (2d72a8894cfb609ee616b098e165c722183ba325)
Would it be easily possible to squash this commit and the earlier commit "wallet: Remove WriteBestBlock from chainStateFlushed" (35b636469983fdcd5f8e62448f4733effe596e41) together?
I feel like that wouild make the PR easier to understand because the earlier commit doesn't provide any rationale while the commit message here is a little misleading because the commit at this point is only removing dead cod
...
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2073831535)
In commit "wallet: Replace chainStateFlushed in loading with WriteBestBlock" (8b180358fb4c0585f2d9b8c4239d99a98c1fd663)
Not important, but if a goal of this PR is to try to keep m_last_block_processed and bestblock closer in sync, seems like it would be good to call SetLastBlockProcessed here instead of WriteBestBlock.
Same suggestion also applies to AttachChain below, but that WriteBestBlock to SetLastBlockProcessed replacement is already made in a later commit 08668272030bfcffc78efd77b64
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2073831535)
In commit "wallet: Replace chainStateFlushed in loading with WriteBestBlock" (8b180358fb4c0585f2d9b8c4239d99a98c1fd663)
Not important, but if a goal of this PR is to try to keep m_last_block_processed and bestblock closer in sync, seems like it would be good to call SetLastBlockProcessed here instead of WriteBestBlock.
Same suggestion also applies to AttachChain below, but that WriteBestBlock to SetLastBlockProcessed replacement is already made in a later commit 08668272030bfcffc78efd77b64
...
💬 ryanofsky commented on pull request "wallet: Ensure best block matches wallet scan state":
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2073849511)
In commit "wallet: Write best block record on unload" (eceb848494ea24d3686a9ff9a2d25b657cce9d6e)
Current change should be ok because it's ok if locator is a little behind, but this doesn't seem like most ideal place to write the best block because notifications could still be incoming at the point. A better place would seem like the FlushAndDeleteWallet function before the [Flush](https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/src/wallet/wallet.cpp#L239) call
...
(https://github.com/bitcoin/bitcoin/pull/30221#discussion_r2073849511)
In commit "wallet: Write best block record on unload" (eceb848494ea24d3686a9ff9a2d25b657cce9d6e)
Current change should be ok because it's ok if locator is a little behind, but this doesn't seem like most ideal place to write the best block because notifications could still be incoming at the point. A better place would seem like the FlushAndDeleteWallet function before the [Flush](https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/src/wallet/wallet.cpp#L239) call
...
💬 laanwj commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2851833591)
Same
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
164958d8c5292ef876e3454725888a488565a414af690a568caedae31215db6c guix-build-b074ae1b4f52/output/dist-archive/bitcoin-b074ae1b4f52.tar.gz
145be7d02fd3d967351ad5944ffe4ceb885e3f7bed6677be657b1c0aa125b40c guix-build-b074ae1b4f52/output/x86_64-w64-mingw32/SHA256SUMS.part
2794ba8c24e64ca1068e04b73748e1406df51089a149a8b0a70637d0bf8e120a guix-build-b074ae1b4f52/output/x
...
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2851833591)
Same
```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
164958d8c5292ef876e3454725888a488565a414af690a568caedae31215db6c guix-build-b074ae1b4f52/output/dist-archive/bitcoin-b074ae1b4f52.tar.gz
145be7d02fd3d967351ad5944ffe4ceb885e3f7bed6677be657b1c0aa125b40c guix-build-b074ae1b4f52/output/x86_64-w64-mingw32/SHA256SUMS.part
2794ba8c24e64ca1068e04b73748e1406df51089a149a8b0a70637d0bf8e120a guix-build-b074ae1b4f52/output/x
...
📝 theStack opened a pull request: "test: refactor: overhaul (w)txid determination for `CTransaction` objects"
(https://github.com/bitcoin/bitcoin/pull/32421)
In the functional test framework, determining a (w)txid for a `CTransaction` instance is currently rather confusing and footgunny due to inconsistent naming/interfaces (see table below) and statefulness involved. This PR aims to improve that by:
* removing the (w)txid caching mechanism, in order to avoid the need to call additional rehashing functions (`.rehash()`/`.calculate_sha256()`, see first two commits and https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993286997). This change
...
(https://github.com/bitcoin/bitcoin/pull/32421)
In the functional test framework, determining a (w)txid for a `CTransaction` instance is currently rather confusing and footgunny due to inconsistent naming/interfaces (see table below) and statefulness involved. This PR aims to improve that by:
* removing the (w)txid caching mechanism, in order to avoid the need to call additional rehashing functions (`.rehash()`/`.calculate_sha256()`, see first two commits and https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993286997). This change
...
🤔 vasild reviewed a pull request: "test: add test for malleated transaction with valid witness"
(https://github.com/bitcoin/bitcoin/pull/32385#pullrequestreview-2815650001)
Concept ACK, thank you for looking into this!
(https://github.com/bitcoin/bitcoin/pull/32385#pullrequestreview-2815650001)
Concept ACK, thank you for looking into this!
💬 vasild commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073927377)
I can imagine that malleating a transaction does not require the private key and a re-sign.
Currently there is this function:
```py
def create_malleated_version(self, tx)
```
which fills garbage. Could it be extended to take a boolean argument, indicating whether to fill with garbage like now, or flip `s` to `ORDER - s`? For this it would have to extract `s` like done on line 97 here:
https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/test/functional/t
...
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073927377)
I can imagine that malleating a transaction does not require the private key and a re-sign.
Currently there is this function:
```py
def create_malleated_version(self, tx)
```
which fills garbage. Could it be extended to take a boolean argument, indicating whether to fill with garbage like now, or flip `s` to `ORDER - s`? For this it would have to extract `s` like done on line 97 here:
https://github.com/bitcoin/bitcoin/blob/baa848b8d38187ce6b24a57cfadf1ea180209711/test/functional/t
...
💬 vasild commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073931876)
Isn't multisig kind of out-of-scope for creating a valid malleated transaction? I don't mind some extra tests, but those might make this PR more difficult to review. Maybe omit those if there is no review interest for some time.
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073931876)
Isn't multisig kind of out-of-scope for creating a valid malleated transaction? I don't mind some extra tests, but those might make this PR more difficult to review. Maybe omit those if there is no review interest for some time.
💬 instagibbs commented on pull request "test: refactor: overhaul (w)txid determination for `CTransaction` objects":
(https://github.com/bitcoin/bitcoin/pull/32421#issuecomment-2851921477)
> determining a (w)txid for a CTransaction instance is currently rather confusing and footgunny
I don't disbelieve you exactly as I always have to double-check what I'm calling, but have we seen a series of footguns?
(https://github.com/bitcoin/bitcoin/pull/32421#issuecomment-2851921477)
> determining a (w)txid for a CTransaction instance is currently rather confusing and footgunny
I don't disbelieve you exactly as I always have to double-check what I'm calling, but have we seen a series of footguns?
🤔 murchandamus reviewed a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2815722815)
I concept ACKed this PR earlier, but after discussing the proposed change for several days, I would prefer if the configuration option were removed at a later time. I still support dropping the limit on count and size of OP_RETURN data payloads.
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2815722815)
I concept ACKed this PR earlier, but after discussing the proposed change for several days, I would prefer if the configuration option were removed at a later time. I still support dropping the limit on count and size of OP_RETURN data payloads.
💬 stratospher commented on pull request "test: add test for malleated transaction with valid witness":
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073973241)
oh included it because high-s transactions are non-standard and I was worried they wouldn't be relayed and we couldn't test private broadcast behaviour.
(https://github.com/bitcoin/bitcoin/pull/32385#discussion_r2073973241)
oh included it because high-s transactions are non-standard and I was worried they wouldn't be relayed and we couldn't test private broadcast behaviour.
💬 luke-jr commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851998711)
Concept NACK. Already been over this on multiple PRs. Spamming it with immaterial minor differences won't change the fundamental insanity and malice of the concept.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851998711)
Concept NACK. Already been over this on multiple PRs. Spamming it with immaterial minor differences won't change the fundamental insanity and malice of the concept.
💬 1440000bytes commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851999012)
@moth-oss config options are not removed in this pull request.
(https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2851999012)
@moth-oss config options are not removed in this pull request.
👍 laanwj approved a pull request: "cmake: Add application manifests when cross-compiling for Windows"
(https://github.com/bitcoin/bitcoin/pull/32396#pullrequestreview-2815767493)
ACK b074ae1b4f52471a9d71e9431deebae5ec3d603e
Have only tested building with guix (where the symbol-check verifies that the manifests are correctly added), not the native Windows build.
(https://github.com/bitcoin/bitcoin/pull/32396#pullrequestreview-2815767493)
ACK b074ae1b4f52471a9d71e9431deebae5ec3d603e
Have only tested building with guix (where the symbol-check verifies that the manifests are correctly added), not the native Windows build.
💬 hebasto commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2852019377)
Friendly ping @davidgumberg @hodlinator @sipsorcery :)
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2852019377)
Friendly ping @davidgumberg @hodlinator @sipsorcery :)
💬 cmdruid commented on pull request "[WIP] rpc: add `clearmempool` command for regtest mode":
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2852150995)
> What about invalidating the block, recording all mempool txs via `getrawmempool`, stopping the node, deleting `mempool.dat` and restarting?
The wallets will reinsert the transactions unless you prevent them from doing so, but then you don't know what transactions to remove from the wallet. Restarting the node mid-test is also slow and annoying.
> Previously proposed in https://github.com/bitcoin/bitcoin/pull/13836
I missed this earlier 😬, thank you for linking it.
The 2018 codebas
...
(https://github.com/bitcoin/bitcoin/pull/32418#issuecomment-2852150995)
> What about invalidating the block, recording all mempool txs via `getrawmempool`, stopping the node, deleting `mempool.dat` and restarting?
The wallets will reinsert the transactions unless you prevent them from doing so, but then you don't know what transactions to remove from the wallet. Restarting the node mid-test is also slow and annoying.
> Previously proposed in https://github.com/bitcoin/bitcoin/pull/13836
I missed this earlier 😬, thank you for linking it.
The 2018 codebas
...
📝 darosior opened a pull request: "spam: trick Drahtbot into rendering a scammy link"
(https://github.com/bitcoin/bitcoin/pull/32422)
Since this LLM experimental feature was rolled out on DrahtBot i noticed it would not sanitize inputs and render Markdown. So i'm wondering if i intentionally introduced a Python comment with a typo in it that points to a scam website, i could get Drahtbot to render a big bold scam link.
This is my attempt, sorry for the noise.
(https://github.com/bitcoin/bitcoin/pull/32422)
Since this LLM experimental feature was rolled out on DrahtBot i noticed it would not sanitize inputs and render Markdown. So i'm wondering if i intentionally introduced a Python comment with a typo in it that points to a scam website, i could get Drahtbot to render a big bold scam link.
This is my attempt, sorry for the noise.