💬 Sjors commented on pull request "interface: Expose load utxo snapshot functionality":
(https://github.com/bitcoin-core/gui/pull/869#issuecomment-2837856588)
@D33r-Gee I'm a bit confused what the plan is here. Do you intend to implement this feature in the current GUI, or only in the QML project?
In the plan is to implement it here, which would be great, I would suggest opening two pull requests:
1. The entire feature, including the interface change commit, here - but marked as draft
2. The interface change commit, against the Bitcoin Core repo. Conceptually this in an interface ON the node FOR the GUI, so it belong in that repo. From there yo
...
(https://github.com/bitcoin-core/gui/pull/869#issuecomment-2837856588)
@D33r-Gee I'm a bit confused what the plan is here. Do you intend to implement this feature in the current GUI, or only in the QML project?
In the plan is to implement it here, which would be great, I would suggest opening two pull requests:
1. The entire feature, including the interface change commit, here - but marked as draft
2. The interface change commit, against the Bitcoin Core repo. Conceptually this in an interface ON the node FOR the GUI, so it belong in that repo. From there yo
...
💬 Sjors commented on pull request "validation: log which peer sent us a header":
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-2837862858)
@mzumsande does that diagram look right to you?
(https://github.com/bitcoin/bitcoin/pull/27826#issuecomment-2837862858)
@mzumsande does that diagram look right to you?
💬 wizkid057 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837886136)
> Anything that can appear in your mempool via `OP_RETURN` can already appear as an inscription, at 1/4th the cost for the attacker.
Incorrect. Because 'inscriptions' are an exploit, they have to break up that data into ~500 byte chunks partitioned by PUSH opcodes. This means the largest contiguous piece of raw data to contend with is ~500 bytes.
With unlimited sized OP_RETURN, we now have to deal with ~100KB contiguous pieces of data... 200x more. Far more dangerous from an external anal
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837886136)
> Anything that can appear in your mempool via `OP_RETURN` can already appear as an inscription, at 1/4th the cost for the attacker.
Incorrect. Because 'inscriptions' are an exploit, they have to break up that data into ~500 byte chunks partitioned by PUSH opcodes. This means the largest contiguous piece of raw data to contend with is ~500 bytes.
With unlimited sized OP_RETURN, we now have to deal with ~100KB contiguous pieces of data... 200x more. Far more dangerous from an external anal
...
💬 Turtlecute33 commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837901444)
Hey, I’d like to ask why my comment above (https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2801335235) was marked as a duplicate. I believe it includes some technical points that weren’t cited earlier, for example, that allowing more OP_RETURN usage doesn’t necessarily mean spammers will stop using witness-discounted Taproot outputs.
(cc @pinheadmz)
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837901444)
Hey, I’d like to ask why my comment above (https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2801335235) was marked as a duplicate. I believe it includes some technical points that weren’t cited earlier, for example, that allowing more OP_RETURN usage doesn’t necessarily mean spammers will stop using witness-discounted Taproot outputs.
(cc @pinheadmz)
🤔 BrandonOdiwuor reviewed a pull request: "test: Use the correct node for doubled keypath test"
(https://github.com/bitcoin/bitcoin/pull/32369#pullrequestreview-2802477132)
Code Review ACK 32d55e28af69ef09094ba921289bf4d1ad79905a
Confirmed that `node 5(8-3)` with the correct configs for v22.0 is being used
https://github.com/bitcoin/bitcoin/blob/65714c162c169cee7d95cf1fb2a9decfa8f9f9ad/test/functional/wallet_backwards_compatibility.py#L42
(https://github.com/bitcoin/bitcoin/pull/32369#pullrequestreview-2802477132)
Code Review ACK 32d55e28af69ef09094ba921289bf4d1ad79905a
Confirmed that `node 5(8-3)` with the correct configs for v22.0 is being used
https://github.com/bitcoin/bitcoin/blob/65714c162c169cee7d95cf1fb2a9decfa8f9f9ad/test/functional/wallet_backwards_compatibility.py#L42
💬 polespinasa commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837920532)
> allowing more OP_RETURN usage doesn’t necessarily mean spammers will stop using witness-discounted Taproot outputs
That's in the mailing list, not that direct sentence but cost comparison with witness-discount and multiple op_returns use cases.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837920532)
> allowing more OP_RETURN usage doesn’t necessarily mean spammers will stop using witness-discounted Taproot outputs
That's in the mailing list, not that direct sentence but cost comparison with witness-discount and multiple op_returns use cases.
🚀 fanquake merged a pull request: "test: Use the correct node for doubled keypath test"
(https://github.com/bitcoin/bitcoin/pull/32369)
(https://github.com/bitcoin/bitcoin/pull/32369)
💬 ajtowns commented on pull request "RFC: Accept non-std transactions in Testnet4 by default again":
(https://github.com/bitcoin/bitcoin/pull/32133#issuecomment-2838011381)
> If we really wanted Testnet to always behave like mainnet we should have removed the setting too. Next time RSK could just have a connection to a miner who has set it to true and then they don't detect the error and shoot themselves in the foot yet again.
In that case, the same setup that worked for testnet (getting a direct connection to a miner that is willing to support your consensus-valid but non-standard transactions) also works for mainnet, and they haven't shot themselves in the foo
...
(https://github.com/bitcoin/bitcoin/pull/32133#issuecomment-2838011381)
> If we really wanted Testnet to always behave like mainnet we should have removed the setting too. Next time RSK could just have a connection to a miner who has set it to true and then they don't detect the error and shoot themselves in the foot yet again.
In that case, the same setup that worked for testnet (getting a direct connection to a miner that is willing to support your consensus-valid but non-standard transactions) also works for mainnet, and they haven't shot themselves in the foo
...
🤔 janb84 reviewed a pull request: "cmake: Check user-defined `APPEND_*FLAGS` variables early"
(https://github.com/bitcoin/bitcoin/pull/32367#pullrequestreview-2802672115)
ACK [eb2a943](https://github.com/bitcoin/bitcoin/commit/eb2a9435044ac91442fafc606c5af2f473bff3c5)
- PR fixes #32323 (reproduced on main, checked it was solved by this PR) ✅
- Code review looks good to me. Adds a new macro with additional checks and some optimisations.✅ (There is one reference to `enable_languages()` in secp256k1 but that is separate and not relevant to this PR )
<details>
### Main:
Configure with faulty flag:
`cmake -B build -DAPPEND_CXXFLAGS="-ftrivial-auto-var-i
...
(https://github.com/bitcoin/bitcoin/pull/32367#pullrequestreview-2802672115)
ACK [eb2a943](https://github.com/bitcoin/bitcoin/commit/eb2a9435044ac91442fafc606c5af2f473bff3c5)
- PR fixes #32323 (reproduced on main, checked it was solved by this PR) ✅
- Code review looks good to me. Adds a new macro with additional checks and some optimisations.✅ (There is one reference to `enable_languages()` in secp256k1 but that is separate and not relevant to this PR )
<details>
### Main:
Configure with faulty flag:
`cmake -B build -DAPPEND_CXXFLAGS="-ftrivial-auto-var-i
...
⚠️ Sjors opened an issue: "XOR existing mempools"
(https://github.com/bitcoin/bitcoin/issues/32372)
#32359 allows for larger contiguous payloads to end up in the mempool than what can be achieved with inscriptions. This might trip a naive virus scanner into false positives.
It seems like a good precaution to XOR existing mempools, rather than only new mempools. Assuming this feature has been tested well enough by now, a migration seems relatively safe.
(https://github.com/bitcoin/bitcoin/issues/32372)
#32359 allows for larger contiguous payloads to end up in the mempool than what can be achieved with inscriptions. This might trip a naive virus scanner into false positives.
It seems like a good precaution to XOR existing mempools, rather than only new mempools. Assuming this feature has been tested well enough by now, a migration seems relatively safe.
💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2838068278)
> [Inscriptions] have to break up that data into ~500 byte chunks partitioned by PUSH opcodes. This means the largest contiguous piece of raw data to contend with is ~500 bytes.
> With unlimited sized `OP_RETURN`, we now have to deal with ~100KB contiguous pieces of data... 200x more. Far more dangerous from an external analysis standpoint.
It would be a reasonable precaution to pre-emptively XOR existing mempools by the next release. Tracking in #32372.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2838068278)
> [Inscriptions] have to break up that data into ~500 byte chunks partitioned by PUSH opcodes. This means the largest contiguous piece of raw data to contend with is ~500 bytes.
> With unlimited sized `OP_RETURN`, we now have to deal with ~100KB contiguous pieces of data... 200x more. Far more dangerous from an external analysis standpoint.
It would be a reasonable precaution to pre-emptively XOR existing mempools by the next release. Tracking in #32372.
👍 vasild approved a pull request: "p2p: Advance pindexLastCommonBlock early after connecting blocks"
(https://github.com/bitcoin/bitcoin/pull/32180#pullrequestreview-2802709072)
ACK b7ff6a611a220e9380f6cd6428f1d3483c8d566f
> But if peer0 is processed twice in a row (which can happen due to the randomization in ThreadMessageHandler), we could request block 1025 from it
I confirm this, made it reproducible:
* `git show 855b1fc2b9 |git apply -R`
* observe `p2p_ibd_stalling.py` is failing (mostly).
* apply the patch below to mess with the order in which nodes are processed
* observe `p2p_ibd_stalling.py` succeeding
<details>
<summary>[patch] change order in wh
...
(https://github.com/bitcoin/bitcoin/pull/32180#pullrequestreview-2802709072)
ACK b7ff6a611a220e9380f6cd6428f1d3483c8d566f
> But if peer0 is processed twice in a row (which can happen due to the randomization in ThreadMessageHandler), we could request block 1025 from it
I confirm this, made it reproducible:
* `git show 855b1fc2b9 |git apply -R`
* observe `p2p_ibd_stalling.py` is failing (mostly).
* apply the patch below to mess with the order in which nodes are processed
* observe `p2p_ibd_stalling.py` succeeding
<details>
<summary>[patch] change order in wh
...
⚠️ Sjors opened an issue: "rfc: only relay transactions to v2 encrypted peers"
(https://github.com/bitcoin/bitcoin/issues/32373)
Unless we don't have any.
One of the goals of p2p traffic encryption was to make it impossible for passive observers to figure out where a transaction originates. However as long as there's enough unencrypted connections remaining, that doesn't seem very effective.
Assuming v2 transport adoption has reached an acceptable level, it may be a good time to start preferring it more strongly.
Specifically I would suggest that we only relay transactions to v2 peers.
If a v1 peer asks for a transact
...
(https://github.com/bitcoin/bitcoin/issues/32373)
Unless we don't have any.
One of the goals of p2p traffic encryption was to make it impossible for passive observers to figure out where a transaction originates. However as long as there's enough unencrypted connections remaining, that doesn't seem very effective.
Assuming v2 transport adoption has reached an acceptable level, it may be a good time to start preferring it more strongly.
Specifically I would suggest that we only relay transactions to v2 peers.
If a v1 peer asks for a transact
...
💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2838139448)
Rebased to cover new tests to resolve new CI failure.
Also adjusted the test comments to mention heap/stack less often.
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-2838139448)
Rebased to cover new tests to resolve new CI failure.
Also adjusted the test comments to mention heap/stack less often.
💬 Sjors commented on issue "rfc: only relay transactions to v2 encrypted peers":
(https://github.com/bitcoin/bitcoin/issues/32373#issuecomment-2838158039)
Related, but more radical: #29618
There's also #29415, but that's limited in scope to our own transactions and relies on Tor / I2P.
(https://github.com/bitcoin/bitcoin/issues/32373#issuecomment-2838158039)
Related, but more radical: #29618
There's also #29415, but that's limited in scope to our own transactions and relies on Tor / I2P.
💬 l0rinc commented on pull request "coins: Fix `cachedCoinsUsage` usages in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-2838158337)
Rebased to allow CI running the new tests.
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-2838158337)
Rebased to allow CI running the new tests.
💬 fanquake commented on pull request "build: enable libc++ hardening in debug builds":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2838173912)
`ENABLE_HARDENING` and the `hardening_interface` have been removed.
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2838173912)
`ENABLE_HARDENING` and the `hardening_interface` have been removed.
💬 Sjors commented on pull request "Remove the legacy wallet and BDB dependency":
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2838186737)
The `updateWatchOnlyLabels` stuff is still wrong. I think the entire second column for watch-only needs to be stripped from the GUI. But at least not rendered.
(https://github.com/bitcoin/bitcoin/pull/28710#issuecomment-2838186737)
The `updateWatchOnlyLabels` stuff is still wrong. I think the entire second column for watch-only needs to be stripped from the GUI. But at least not rendered.
💬 jlopp commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2838236723)
@wizkid057 I think you still need to provide further clarification
> We're going from a landscape with tolerated data carrying of ~80 bytes alongside an unpatched exploit that can stuff ~500 bytes per chunk disguised as a script.... to keeping the exploit AND adding a method for storing data up to ~100KB in length contiguously... and I'm one of the only ones here with the foresight to know that this will have undermining consequences to the ecosystem as a whole?
What is the exploit you are
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2838236723)
@wizkid057 I think you still need to provide further clarification
> We're going from a landscape with tolerated data carrying of ~80 bytes alongside an unpatched exploit that can stuff ~500 bytes per chunk disguised as a script.... to keeping the exploit AND adding a method for storing data up to ~100KB in length contiguously... and I'm one of the only ones here with the foresight to know that this will have undermining consequences to the ecosystem as a whole?
What is the exploit you are
...
💬 laanwj commented on issue "windows: usage of deprecated `std:wstring_convert`":
(https://github.com/bitcoin/bitcoin/issues/32361#issuecomment-2838255992)
> Note that subprocess.h is also using wstring_convert:
Ugh. Well at least it's an upstream issue for them, too, then.
(https://github.com/bitcoin/bitcoin/issues/32361#issuecomment-2838255992)
> Note that subprocess.h is also using wstring_convert:
Ugh. Well at least it's an upstream issue for them, too, then.