💬 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.
💬 laanwj commented on issue "windows: usage of deprecated `std:wstring_convert`":
(https://github.com/bitcoin/bitcoin/issues/32361#issuecomment-2838264061)
So in principle modern Windows can use UTF-8 on the 8-bit APIs, removing need for any conversion, but i'm not sure starting from when, and this relies on setting a specific code page globally at startup.
Alternatively we could borrow these (windows specific) functions from our leveldb patch:
```
// Converts a Windows wide multi-byte UTF-16 string to a UTF-8 string.
// See http://utf8everywhere.org/#windows
std::string toUtf8(const std::wstring& wstr) {
if (wstr.empty()) return std::st
...
(https://github.com/bitcoin/bitcoin/issues/32361#issuecomment-2838264061)
So in principle modern Windows can use UTF-8 on the 8-bit APIs, removing need for any conversion, but i'm not sure starting from when, and this relies on setting a specific code page globally at startup.
Alternatively we could borrow these (windows specific) functions from our leveldb patch:
```
// Converts a Windows wide multi-byte UTF-16 string to a UTF-8 string.
// See http://utf8everywhere.org/#windows
std::string toUtf8(const std::wstring& wstr) {
if (wstr.empty()) return std::st
...
👍 brunoerg approved a pull request: "descriptors: Reject + sign while parsing unsigned"
(https://github.com/bitcoin/bitcoin/pull/32365#pullrequestreview-2802901832)
code review ACK fa655da159876861251d1149a5c31a830bd35596
(https://github.com/bitcoin/bitcoin/pull/32365#pullrequestreview-2802901832)
code review ACK fa655da159876861251d1149a5c31a830bd35596
💬 laanwj commented on issue "XOR existing mempools":
(https://github.com/bitcoin/bitcoin/issues/32372#issuecomment-2838311332)
Why don't we? Isn't the `mempool.dat` always written from scratch and not changed in place?
(https://github.com/bitcoin/bitcoin/issues/32372#issuecomment-2838311332)
Why don't we? Isn't the `mempool.dat` always written from scratch and not changed in place?
💬 rot13maxi commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2838382161)
> > 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 exte
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2838382161)
> > 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 exte
...
💬 hebasto commented on issue "Option to use dark theme for Windows":
(https://github.com/bitcoin-core/gui/issues/378#issuecomment-2838429843)
Also https://bitcoin.stackexchange.com/questions/126170/bitcoin-core-development
(https://github.com/bitcoin-core/gui/issues/378#issuecomment-2838429843)
Also https://bitcoin.stackexchange.com/questions/126170/bitcoin-core-development
💬 theDavidCoen commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2838436958)
I agree with Pieter Wuille when he wrotes: "(...) these arguments do not apply to OP_RETURN limits, which don't serve an objective harm reduction beyond a subjective "that isn't what you should be using the chain for".
So yes, maybe OP_RETURN limits do not work to prevent spam and we can remove the default value that is totally arbitrary, but node policies are subjective by definition and, as a node operator, I should be allow to define my own size limit without recurring to a fork of Bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2838436958)
I agree with Pieter Wuille when he wrotes: "(...) these arguments do not apply to OP_RETURN limits, which don't serve an objective harm reduction beyond a subjective "that isn't what you should be using the chain for".
So yes, maybe OP_RETURN limits do not work to prevent spam and we can remove the default value that is totally arbitrary, but node policies are subjective by definition and, as a node operator, I should be allow to define my own size limit without recurring to a fork of Bitcoin
...
⚠️ fanquake opened an issue: "test: interface_usdt_net.py failure under --valgrind"
(https://github.com/bitcoin/bitcoin/issues/32374)
Ubuntu 24.04.2 LTS (GNU/Linux 6.8.0-58-generic x86_64)
gcc (Ubuntu 14.2.0-4ubuntu2~24.04) 14.2.0
valgrind-3.25.0
```bash
cmake -B build -DWITH_USDT=ON
cmake --build build
./build/test/functional/test_runner.py --timeout-factor=9999 --valgrind interface_usdt_net.py
```
```bash
/root/ci_scratch/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20250429_111832/interface_usdt_net_0'
test 2025-04-29T11:18:32.583000Z TestFramework (INFO): PRNG seed is: 3961365374660180483
test 2025-04-29T1
...
(https://github.com/bitcoin/bitcoin/issues/32374)
Ubuntu 24.04.2 LTS (GNU/Linux 6.8.0-58-generic x86_64)
gcc (Ubuntu 14.2.0-4ubuntu2~24.04) 14.2.0
valgrind-3.25.0
```bash
cmake -B build -DWITH_USDT=ON
cmake --build build
./build/test/functional/test_runner.py --timeout-factor=9999 --valgrind interface_usdt_net.py
```
```bash
/root/ci_scratch/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20250429_111832/interface_usdt_net_0'
test 2025-04-29T11:18:32.583000Z TestFramework (INFO): PRNG seed is: 3961365374660180483
test 2025-04-29T1
...