📝 vasild converted_to_draft a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326)
`CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.
Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the
...
(https://github.com/bitcoin/bitcoin/pull/32326)
`CConnman::FindNode()` would lock `m_nodes_mutex`, find the node in `m_nodes`, release the mutex and return the node. The current code is safe but it is a dangerous interface where a caller may end up using the node returned from `FindNode()` without owning `m_nodes_mutex` and without having that node's reference count incremented.
Change `FindNode()` to return a boolean since all but one of its callers used its return value to check whether a node exists and did not do anything else with the
...
💬 vasild commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2837679823)
Converted to draft, waiting for:
* https://github.com/bitcoin/bitcoin/pull/32338 to make it
* then change this PR to use naming as in https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2063650801
Further improvements away from this PR:
* make `m_nodes_mutex` non-recursive, https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2835048806
* assess all the checks that compare just the address or address+port and remove redundant ones and make the calls consistent regardless of
...
(https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2837679823)
Converted to draft, waiting for:
* https://github.com/bitcoin/bitcoin/pull/32338 to make it
* then change this PR to use naming as in https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2063650801
Further improvements away from this PR:
* make `m_nodes_mutex` non-recursive, https://github.com/bitcoin/bitcoin/pull/32326#issuecomment-2835048806
* assess all the checks that compare just the address or address+port and remove redundant ones and make the calls consistent regardless of
...
💬 shahsb commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837703405)
It's great to see constructive discussions taking place here!
I don't see any valid reason to take away configuration settings from users.
Just because certain config options don't directly influence what miners include in blocks doesn't mean users lose control over what appears in their mempools.
Therefore, my take is: Concept NACK.
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2837703405)
It's great to see constructive discussions taking place here!
I don't see any valid reason to take away configuration settings from users.
Just because certain config options don't directly influence what miners include in blocks doesn't mean users lose control over what appears in their mempools.
Therefore, my take is: Concept NACK.
👍 Sjors approved a pull request: "Remove arbitrary limits on OP_Return (datacarrier) outputs"
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2802349810)
ACK 47e713ea5a96d3bb2ddd64c8a87607e5ccea8c72
Thanks for adding the tests.
Nits:
- maybe rename the first commit title to: "Drop OP_RETURN standardness limits for datacarrier size and output count"-
- squash 47e713ea5a96d3bb2ddd64c8a87607e5ccea8c72 into it
- see inline comment for test improvement
---
> Considering the floodgates of absolutely horror this PR is going to bring upon Bitcoin node runners
Anything that can appear in your mempool via `OP_RETURN` can already appear as
...
(https://github.com/bitcoin/bitcoin/pull/32359#pullrequestreview-2802349810)
ACK 47e713ea5a96d3bb2ddd64c8a87607e5ccea8c72
Thanks for adding the tests.
Nits:
- maybe rename the first commit title to: "Drop OP_RETURN standardness limits for datacarrier size and output count"-
- squash 47e713ea5a96d3bb2ddd64c8a87607e5ccea8c72 into it
- see inline comment for test improvement
---
> Considering the floodgates of absolutely horror this PR is going to bring upon Bitcoin node runners
Anything that can appear in your mempool via `OP_RETURN` can already appear as
...
💬 Sjors commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2065694091)
In 8fd09d622d161eb554f080a52735832119cb35e5 "Add more OP_RETURN mempool acceptance functional tests"
This this is now a bit redundant, so might as well expand it to clarify something:
```diff
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
index a4dfda8443..2ac4905a57 100755
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -338,9 +338,10 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
)
#
...
(https://github.com/bitcoin/bitcoin/pull/32359#discussion_r2065694091)
In 8fd09d622d161eb554f080a52735832119cb35e5 "Add more OP_RETURN mempool acceptance functional tests"
This this is now a bit redundant, so might as well expand it to clarify something:
```diff
diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
index a4dfda8443..2ac4905a57 100755
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py
@@ -338,9 +338,10 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
)
#
...
💬 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.