💬 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.
✅ darosior closed a pull request: "spam: trick Drahtbot into rendering a scammy link"
(https://github.com/bitcoin/bitcoin/pull/32422)
(https://github.com/bitcoin/bitcoin/pull/32422)
💬 darosior commented on pull request "spam: trick Drahtbot into rendering a scammy link":
(https://github.com/bitcoin/bitcoin/pull/32422#issuecomment-2852185631)
Ugh it didn't even work. In this instance it did render it! https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2847530907
(https://github.com/bitcoin/bitcoin/pull/32422#issuecomment-2852185631)
Ugh it didn't even work. In this instance it did render it! https://github.com/bitcoin/bitcoin/pull/32406#issuecomment-2847530907
💬 purpleKarrot commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2074116183)
Would it be possible to put `get_directory_property(precious_variables CACHE_VARIABLES)` here inside the function scope?
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2074116183)
Would it be possible to put `get_directory_property(precious_variables CACHE_VARIABLES)` here inside the function scope?
💬 purpleKarrot commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2074117354)
```suggestion
string(TOUPPER "CMAKE_CXX_FLAGS_${config}" var_name)
```
(https://github.com/bitcoin/bitcoin/pull/32356#discussion_r2074117354)
```suggestion
string(TOUPPER "CMAKE_CXX_FLAGS_${config}" var_name)
```
💬 jonatack commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2074128855)
I don't have a strong opinion on removing the versions here; either way can probably have just one commit, e.g. "update dependency versions" or "remove versions"
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2074128855)
I don't have a strong opinion on removing the versions here; either way can probably have just one commit, e.g. "update dependency versions" or "remove versions"
🤔 glozow reviewed a pull request: "p2p: stop special-casing witness-stripped error for unconfirmed transactions"
(https://github.com/bitcoin/bitcoin/pull/32379#pullrequestreview-2816057986)
From the offline conversation, here's a [branch](https://github.com/glozow/bitcoin/tree/2025-05-rejected-parents) that drops the "don't resolve orphans with a parent in RecentRejects" logic, meaning we'll still keep an orphan of a witness stripped-transaction after this PR. This doesn't fully fix the fact that 1p1c is disrupted, because the parent is still not requested (its txid is AlreadyHave). However, keeping the orphan means we still accept the 1p1c if we receive the parent via wtxid.
Al
...
(https://github.com/bitcoin/bitcoin/pull/32379#pullrequestreview-2816057986)
From the offline conversation, here's a [branch](https://github.com/glozow/bitcoin/tree/2025-05-rejected-parents) that drops the "don't resolve orphans with a parent in RecentRejects" logic, meaning we'll still keep an orphan of a witness stripped-transaction after this PR. This doesn't fully fix the fact that 1p1c is disrupted, because the parent is still not requested (its txid is AlreadyHave). However, keeping the orphan means we still accept the 1p1c if we receive the parent via wtxid.
Al
...
💬 hodlinator commented on pull request "doc: Improve `dependencies.md`":
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2074188557)
> I don't have a strong opinion on removing the versions here; either way can probably have just one commit, e.g. "update dependency versions" or "remove versions"
+1
(https://github.com/bitcoin/bitcoin/pull/31895#discussion_r2074188557)
> I don't have a strong opinion on removing the versions here; either way can probably have just one commit, e.g. "update dependency versions" or "remove versions"
+1
💬 laanwj commented on pull request "qt, wallet: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2852352421)
> Would be good to register this new type into the Qt's meta-object system. Look at the RegisterMetaTypes() function on src/qt/bitcoin.cpp.
Good point. It's used in at least the `TransactionView::bumpedFee` signal, which might be broken now.
It could be that adding only `Q_DECLARE_METATYPE(Txid)` at the top is enough as this was enough for uint256.
(https://github.com/bitcoin/bitcoin/pull/32238#issuecomment-2852352421)
> Would be good to register this new type into the Qt's meta-object system. Look at the RegisterMetaTypes() function on src/qt/bitcoin.cpp.
Good point. It's used in at least the `TransactionView::bumpedFee` signal, which might be broken now.
It could be that adding only `Q_DECLARE_METATYPE(Txid)` at the top is enough as this was enough for uint256.
🤔 Psifour reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2816051010)
ACK with 1 nit
My issue with previous PR was largely due to removal vs deprecation of config options.
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2816051010)
ACK with 1 nit
My issue with previous PR was largely due to removal vs deprecation of config options.
💬 Psifour commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2074163691)
agreed on this nit.
It is not immediately apparent that `max_datacarrier_bytes` will be set to `MAX_OP_RETURN_RELAY` if no `-datacarriersize` flag is present.
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2074163691)
agreed on this nit.
It is not immediately apparent that `max_datacarrier_bytes` will be set to `MAX_OP_RETURN_RELAY` if no `-datacarriersize` flag is present.
💬 pablomartin4btc commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#discussion_r2074253242)
> the for after `// Unload the wallets`...
There's the removal of the wallet from the context (`if (!RemoveWallet(context, w, /*load_on_start=*/false)) {`) using `created_wallets` - that was done when the wallet was loaded (in `LoadWalletInternal`-> `AddWallet(context, wallet);` ).
> the lines after `// Verify that there is no dangling wallet` which needs `ptr_wallet`.
At the end of the condition is returning an error (`return util::Error{error};`)
Be aware that if you remove the com
...
(https://github.com/bitcoin/bitcoin/pull/29694#discussion_r2074253242)
> the for after `// Unload the wallets`...
There's the removal of the wallet from the context (`if (!RemoveWallet(context, w, /*load_on_start=*/false)) {`) using `created_wallets` - that was done when the wallet was loaded (in `LoadWalletInternal`-> `AddWallet(context, wallet);` ).
> the lines after `// Verify that there is no dangling wallet` which needs `ptr_wallet`.
At the end of the condition is returning an error (`return util::Error{error};`)
Be aware that if you remove the com
...