🚀 fanquake merged a pull request: "tracing: fix invalid argument in mempool_monitor"
(https://github.com/bitcoin/bitcoin/pull/32454)
(https://github.com/bitcoin/bitcoin/pull/32454)
💬 hMsats commented on issue "Self-compiled bitcoind 29.0 much slower than self-compiled 28.0 on my system":
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2865633695)
@maflcko I will but it will take some time as I'm on vacation right now but will investigate when I have time.
Created `bitcoind` via:
`cmake -B build -DWITH_ZMQ=ON -DBUILD_GUI=ON`
Settings (same as for 28.0):
```
txindex=1
rpcthreads=4
rpcworkqueue=64
dbcache=2500
maxconnections=100
permitbaremultisig=0
datacarrier=0
```
(https://github.com/bitcoin/bitcoin/issues/32455#issuecomment-2865633695)
@maflcko I will but it will take some time as I'm on vacation right now but will investigate when I have time.
Created `bitcoind` via:
`cmake -B build -DWITH_ZMQ=ON -DBUILD_GUI=ON`
Settings (same as for 28.0):
```
txindex=1
rpcthreads=4
rpcworkqueue=64
dbcache=2500
maxconnections=100
permitbaremultisig=0
datacarrier=0
```
💬 fanquake commented on pull request "tracing: fix invalid argument in mempool_monitor":
(https://github.com/bitcoin/bitcoin/pull/32454#issuecomment-2865654520)
Backported to `29.x` in #32292.
(https://github.com/bitcoin/bitcoin/pull/32454#issuecomment-2865654520)
Backported to `29.x` in #32292.
💬 l0rinc commented on issue "qa: Failure in wallet_basic.py spendzeroconfchange test":
(https://github.com/bitcoin/bitcoin/issues/32456#issuecomment-2865716098)
I can't reproduce it either on my Mac - but it does seem to be passing with latest push at least.
<img width="549" alt="Image" src="https://github.com/user-attachments/assets/30b441f9-a8d0-44ff-b23c-2529a7de424a" />
(https://github.com/bitcoin/bitcoin/issues/32456#issuecomment-2865716098)
I can't reproduce it either on my Mac - but it does seem to be passing with latest push at least.
<img width="549" alt="Image" src="https://github.com/user-attachments/assets/30b441f9-a8d0-44ff-b23c-2529a7de424a" />
💬 maflcko commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2865717510)
Not sure if this is worth it. Currently 4 lines of code need to be changed, looking at b537a2c02a9921235d1ecf8c3c7dc1836ec68131. After this, 3 lines will need to be changed. I don't really see the benefit here.
Whereas this comes with downsides:
* People compiling an ancient EOL version/commit will get the impression that they are running a recent release from the current year. This could lead them to thinking they have the latest security patches.
* Conversely, people setting a fixed, bu
...
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2865717510)
Not sure if this is worth it. Currently 4 lines of code need to be changed, looking at b537a2c02a9921235d1ecf8c3c7dc1836ec68131. After this, 3 lines will need to be changed. I don't really see the benefit here.
Whereas this comes with downsides:
* People compiling an ancient EOL version/commit will get the impression that they are running a recent release from the current year. This could lead them to thinking they have the latest security patches.
* Conversely, people setting a fixed, bu
...
💬 captCovalent commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2865777515)
In addition to my post above.
Concept NACK!
At this point, it’s clear that this PR lacks support across many parts of the community. Moving forward despite the current sentiment would cause significant reputational harm to Bitcoin Core. I strongly urge you not to proceed.
The way this proposal has been managed raises concerns about transparency and corruption. Rather than imposing new limitations, we should be enhancing configurability. Satoshi’s vision depends on honest, autonomous nodes—no
...
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2865777515)
In addition to my post above.
Concept NACK!
At this point, it’s clear that this PR lacks support across many parts of the community. Moving forward despite the current sentiment would cause significant reputational harm to Bitcoin Core. I strongly urge you not to proceed.
The way this proposal has been managed raises concerns about transparency and corruption. Rather than imposing new limitations, we should be enhancing configurability. Satoshi’s vision depends on honest, autonomous nodes—no
...
💬 fanquake commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2865786971)
> the behavior just seems more confusing than before for users.
Note that historically we've produced release binaries that have misleading dates:
```bash
Bitcoin Core version v26.1.0
Copyright (C) 2009-2023 The Bitcoin Core developers
```
`26.1` was released in April of 2024: https://github.com/bitcoin/bitcoin/releases/tag/v26.1/. I think `28.1` was the first time we've ever backported a date bump.
> user-facing version numbers or year numbers should just be removed,
As I said abo
...
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2865786971)
> the behavior just seems more confusing than before for users.
Note that historically we've produced release binaries that have misleading dates:
```bash
Bitcoin Core version v26.1.0
Copyright (C) 2009-2023 The Bitcoin Core developers
```
`26.1` was released in April of 2024: https://github.com/bitcoin/bitcoin/releases/tag/v26.1/. I think `28.1` was the first time we've ever backported a date bump.
> user-facing version numbers or year numbers should just be removed,
As I said abo
...
📝 l0rinc opened a pull request: "bench: replace benchmark block with more representative one (413567 → 784588)"
(https://github.com/bitcoin/bitcoin/pull/32457)
#### Summary
This PR replaces our benchmark's reference block with one that's more modern and representative of current usage patterns.
### Context
The [current benchmark block](https://mempool.space/block/413567) was mined in 2016 and added in PR #9049. Since it predates many modern script types, our benchmarks don't accurately reflect current network conditions.
### Suggestion
We're replacing it with [block 784588](https://mempool.space/block/784588) from 2023, which provides a better
...
(https://github.com/bitcoin/bitcoin/pull/32457)
#### Summary
This PR replaces our benchmark's reference block with one that's more modern and representative of current usage patterns.
### Context
The [current benchmark block](https://mempool.space/block/413567) was mined in 2016 and added in PR #9049. Since it predates many modern script types, our benchmarks don't accurately reflect current network conditions.
### Suggestion
We're replacing it with [block 784588](https://mempool.space/block/784588) from 2023, which provides a better
...
💬 maflcko commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2865854548)
> ... 2023 ... `26.1` was released in April of 2024: https://github.com/bitcoin/bitcoin/releases/tag/v26.1/. I think `28.1` was the first time we've ever backported a date bump.
Personally I think it is fine to not bump them for release branches (It is a bit like the Ubuntu release 24.04.2 happening in '25). However, it is also fine to bump the year along with the version. `CMakeLists.txt` will have to be touched anyway for a point release (several times), so touching one line at most once mo
...
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2865854548)
> ... 2023 ... `26.1` was released in April of 2024: https://github.com/bitcoin/bitcoin/releases/tag/v26.1/. I think `28.1` was the first time we've ever backported a date bump.
Personally I think it is fine to not bump them for release branches (It is a bit like the Ubuntu release 24.04.2 happening in '25). However, it is also fine to bump the year along with the version. `CMakeLists.txt` will have to be touched anyway for a point release (several times), so touching one line at most once mo
...
💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081342307)
I don't think it's useful at all, given the historical context. But I don't feel confident enough about dropping it either. That's why I added the historical context in the commit message, so someone else in the future can try dropping it.
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081342307)
I don't think it's useful at all, given the historical context. But I don't feel confident enough about dropping it either. That's why I added the historical context in the commit message, so someone else in the future can try dropping it.
💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2865918066)
> Combining the two in same PR might be better to not miss writing the follow-up.
Could just open an issue to track it?
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2865918066)
> Combining the two in same PR might be better to not miss writing the follow-up.
Could just open an issue to track it?
💬 Sjors commented on pull request "[Policy] Discourage Unsigned Annexes":
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2865947567)
> If desired, tests could be made.
I don't understand annexes at all, so having some tests to illustrate the issue would be handy.
(https://github.com/bitcoin/bitcoin/pull/32453#issuecomment-2865947567)
> If desired, tests could be made.
I don't understand annexes at all, so having some tests to illustrate the issue would be handy.
🤔 l0rinc requested changes to a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2827771860)
I happened to be working on a PR that's related to the exact same code path.
I'm not sure whether the call is redundant or not, but I can understand why we want to avoid situations like https://github.com/bitcoin/bitcoin/pull/9049.
I would prefer a more high-level explanation for the difference between the two calls - explained through code and tests preferably instead of comments - or maybe both.
There are a few minor typos in the commit message as well and it's a bit hand-wavy - can w
...
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2827771860)
I happened to be working on a PR that's related to the exact same code path.
I'm not sure whether the call is redundant or not, but I can understand why we want to avoid situations like https://github.com/bitcoin/bitcoin/pull/9049.
I would prefer a more high-level explanation for the difference between the two calls - explained through code and tests preferably instead of comments - or maybe both.
There are a few minor typos in the commit message as well and it's a bit hand-wavy - can w
...
💬 l0rinc commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081363131)
> it does not count witness and p2sh sigops
Isn't it obvious from the name `Get*Legacy*SigOpCount` and the parameter `GetSigOpCount(bool fAccurate)` called as `GetSigOpCount(false)` that new script types aren't covered and that it's not meant to be accurate but rather compatible with old behavior?
Instead (or maybe next to) the comments, I'd find it more helpful if we documented what the `false` means in `GetLegacySigOpCount`:
```C++
unsigned int GetLegacySigOpCount(const CTransaction& t
...
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081363131)
> it does not count witness and p2sh sigops
Isn't it obvious from the name `Get*Legacy*SigOpCount` and the parameter `GetSigOpCount(bool fAccurate)` called as `GetSigOpCount(false)` that new script types aren't covered and that it's not meant to be accurate but rather compatible with old behavior?
Instead (or maybe next to) the comments, I'd find it more helpful if we documented what the `false` means in `GetLegacySigOpCount`:
```C++
unsigned int GetLegacySigOpCount(const CTransaction& t
...
💬 Sjors commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2865986579)
> as long as they let it completely finish once before starting bitcoind
That seems likely to cause issues. It seems better if we track the height at which xor starts. A script could then work backwards and lower the height. And in that case it might as well be an RPC command.
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2865986579)
> as long as they let it completely finish once before starting bitcoind
That seems likely to cause issues. It seems better if we track the height at which xor starts. A script could then work backwards and lower the height. And in that case it might as well be an RPC command.
💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081395687)
> Isn't it obvious from the name
Given our history of bad function names, no :-)
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2081395687)
> Isn't it obvious from the name
Given our history of bad function names, no :-)
💬 Sjors commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2866037471)
The purpose of this comment isn't to be throrough, it's there to:
1. Warn callers to not rely _only_ on this check
2. For anyone who wants to get rid of this check, point to the relevant context
I think (2) can be achieved with just a commit message and the fact that our merge script points back to the original PR.
I'm not opposed to writing a longer comment, but I suspect it would take a long time to find a text everyone agrees on. And by the time we do, we might as well change the co
...
(https://github.com/bitcoin/bitcoin/pull/31624#issuecomment-2866037471)
The purpose of this comment isn't to be throrough, it's there to:
1. Warn callers to not rely _only_ on this check
2. For anyone who wants to get rid of this check, point to the relevant context
I think (2) can be achieved with just a commit message and the fact that our merge script points back to the original PR.
I'm not opposed to writing a longer comment, but I suspect it would take a long time to find a text everyone agrees on. And by the time we do, we might as well change the co
...
💬 hebasto commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2866040856)
> When someone builds from a tarball that is more than a year old, this will use the current year instead of the year of the release.
If this matters, the following patch could help:
```diff
--- a/.gitattributes
+++ b/.gitattributes
@@ -1 +1,2 @@
+CMakeLists.txt export-subst
src/clientversion.cpp export-subst
diff --git a/CMakeLists.txt b/CMakeLists.txt
index a1665563cb..d0d24db713 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -29,6 +29,7 @@ set(CLIENT_VERSION_BUILD 0)
set
...
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2866040856)
> When someone builds from a tarball that is more than a year old, this will use the current year instead of the year of the release.
If this matters, the following patch could help:
```diff
--- a/.gitattributes
+++ b/.gitattributes
@@ -1 +1,2 @@
+CMakeLists.txt export-subst
src/clientversion.cpp export-subst
diff --git a/CMakeLists.txt b/CMakeLists.txt
index a1665563cb..d0d24db713 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -29,6 +29,7 @@ set(CLIENT_VERSION_BUILD 0)
set
...
💬 laanwj commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2866068613)
> It seems better if we track the height at which xor starts
i really like this idea. "Start XORing from now on" as a default would be enough to prevent new problems, and potentially there could be a background process that updates historical blocks (which would be way less performance critical as no one is waiting for it).
(https://github.com/bitcoin/bitcoin/pull/32451#issuecomment-2866068613)
> It seems better if we track the height at which xor starts
i really like this idea. "Start XORing from now on" as a default would be enough to prevent new problems, and potentially there could be a background process that updates historical blocks (which would be way less performance critical as no one is waiting for it).
💬 Sjors commented on pull request "test: Remove legacy wallet RPC overloads":
(https://github.com/bitcoin/bitcoin/pull/32452#issuecomment-2866112061)
Concept ACK. 2d8679c45107598689cc2af90e6324fa9172b6d3 looks reasonable, though linter is unhappy.
Maybe move b4f5b4e37d29c1a516186775736efda9d77c2fa9 "test: Remove unnecessary importprivkey from wallet_createwallet" to the top and then combine "Add wallet_importprivkey helper for replacing importprivkey" with the commit that replaces the usage. And then also drop `importprivkey` from the `RPCOverloadWrapper`.
(https://github.com/bitcoin/bitcoin/pull/32452#issuecomment-2866112061)
Concept ACK. 2d8679c45107598689cc2af90e6324fa9172b6d3 looks reasonable, though linter is unhappy.
Maybe move b4f5b4e37d29c1a516186775736efda9d77c2fa9 "test: Remove unnecessary importprivkey from wallet_createwallet" to the top and then combine "Add wallet_importprivkey helper for replacing importprivkey" with the commit that replaces the usage. And then also drop `importprivkey` from the `RPCOverloadWrapper`.