🤔 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
...
⚠️ laanwj opened an issue: "Stop shipping ARM32 builds for releases?"
(https://github.com/bitcoin/bitcoin/issues/32375)
Clearly there is no hurry here. This is more of a brainstorming issue than a proposal.
For:
- 64-bit ARM is the standard nowadays for the type of computing hardware our users tend to have (modern Raspberry Pi's and ARM servers). While 32-bit ARM is only still used in low-power embedded systems, an unlikely combination with a bitcoin node.
- 32-bit ARM is the last 32-bit architecture that we ship releases for. After removing this, we could fully focus on optimizing for large virtual address sp
...
(https://github.com/bitcoin/bitcoin/issues/32375)
Clearly there is no hurry here. This is more of a brainstorming issue than a proposal.
For:
- 64-bit ARM is the standard nowadays for the type of computing hardware our users tend to have (modern Raspberry Pi's and ARM servers). While 32-bit ARM is only still used in low-power embedded systems, an unlikely combination with a bitcoin node.
- 32-bit ARM is the last 32-bit architecture that we ship releases for. After removing this, we could fully focus on optimizing for large virtual address sp
...