π¬ darosior commented on pull request "qa: test that we do not disconnect a peer for submitting an invalid compact block":
(https://github.com/bitcoin/bitcoin/pull/33083#issuecomment-3132675457)
Turns out we also didn't test the disconnection logic after a second block building on top of an invalid block is submitted to us (nor the disconnection logic for when an invalid compact block header is submitted to us). This can be checked with:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..f8b9adf910a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1822,7 +1822,7 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid
...
(https://github.com/bitcoin/bitcoin/pull/33083#issuecomment-3132675457)
Turns out we also didn't test the disconnection logic after a second block building on top of an invalid block is submitted to us (nor the disconnection logic for when an invalid compact block header is submitted to us). This can be checked with:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 0c4a89c44cb..f8b9adf910a 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1822,7 +1822,7 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid
...
π¬ fanquake commented on pull request "depends: hard-code necessary c(xx)flags rather than setting them per-host":
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-3132684789)
Guix Build (aarch64):
```bash
d7dcb667b0d801e51490f677fe08be66ba635df93085d0479dcd00443b93bbf2 guix-build-9954d6c83338/output/aarch64-linux-gnu/SHA256SUMS.part
296a26bd69bc458e5a5953876703c664bf760f24ed93c61a43b418522c5c8e3a guix-build-9954d6c83338/output/aarch64-linux-gnu/bitcoin-9954d6c83338-aarch64-linux-gnu-debug.tar.gz
118a81b7e941929f2d95d426e2b2a5d698b90f381819fc68572f5ec06d05b825 guix-build-9954d6c83338/output/aarch64-linux-gnu/bitcoin-9954d6c83338-aarch64-linux-gnu.tar.gz
24d45e
...
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-3132684789)
Guix Build (aarch64):
```bash
d7dcb667b0d801e51490f677fe08be66ba635df93085d0479dcd00443b93bbf2 guix-build-9954d6c83338/output/aarch64-linux-gnu/SHA256SUMS.part
296a26bd69bc458e5a5953876703c664bf760f24ed93c61a43b418522c5c8e3a guix-build-9954d6c83338/output/aarch64-linux-gnu/bitcoin-9954d6c83338-aarch64-linux-gnu-debug.tar.gz
118a81b7e941929f2d95d426e2b2a5d698b90f381819fc68572f5ec06d05b825 guix-build-9954d6c83338/output/aarch64-linux-gnu/bitcoin-9954d6c83338-aarch64-linux-gnu.tar.gz
24d45e
...
π¬ darosior commented on pull request "script: return verification flag responsible for error upon validation failure":
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3132701279)
> In that case you'll already get a network split when the hard fork side mines a block.
Discussed this with AJ (and others). For reference, in this case the disconnection would happen after a second block being submitted on top a first invalid one. We would not disconnect upon a first invalid block being submitted through compact blocks. It turns out that this logic was not tested at all, so i opened https://github.com/bitcoin/bitcoin/pull/33083 which exercises various cases in `MaybePunishN
...
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3132701279)
> In that case you'll already get a network split when the hard fork side mines a block.
Discussed this with AJ (and others). For reference, in this case the disconnection would happen after a second block being submitted on top a first invalid one. We would not disconnect upon a first invalid block being submitted through compact blocks. It turns out that this logic was not tested at all, so i opened https://github.com/bitcoin/bitcoin/pull/33083 which exercises various cases in `MaybePunishN
...
π¬ ryanofsky commented on pull request "kernel: create monolithic kernel static library":
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3132708611)
> Consuming a static library requires a consistent toolchain. .pc files are not reloatable.
@purpleKarrot I read your post the other day and enjoyed it and learned a number of things. And I understand why cmake files are more flexible than .pc files and why they can't support mismatched compilers / libraries and relocation.
But is there something wrong with providing and using .pc files in the case where there is a consistent toolchain and nothing is relocated? This seems like a common & u
...
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3132708611)
> Consuming a static library requires a consistent toolchain. .pc files are not reloatable.
@purpleKarrot I read your post the other day and enjoyed it and learned a number of things. And I understand why cmake files are more flexible than .pc files and why they can't support mismatched compilers / libraries and relocation.
But is there something wrong with providing and using .pc files in the case where there is a consistent toolchain and nothing is relocated? This seems like a common & u
...
β
enirox001 closed an issue: "build: Build warnings from deprecated std::get_temporary_buffer in std::stable_sort calls"
(https://github.com/bitcoin/bitcoin/issues/33080)
(https://github.com/bitcoin/bitcoin/issues/33080)
π¬ maflcko commented on issue "tsan: drop `DEBUG_LOCKORDER` macro from TSAN job?":
(https://github.com/bitcoin/bitcoin/issues/33087#issuecomment-3132725511)
jup, should be fine to drop it, as it is included in the other Debug build type tasks:
* https://cirrus-ci.com/task/5453939128139776?logs=ci#L1086
* https://cirrus-ci.com/task/5876151593205760?logs=ci#L1144
* https://cirrus-ci.com/task/5022920222703616?logs=ci#L1246
* https://cirrus-ci.com/task/6579839034982400?logs=ci#L935
(https://github.com/bitcoin/bitcoin/issues/33087#issuecomment-3132725511)
jup, should be fine to drop it, as it is included in the other Debug build type tasks:
* https://cirrus-ci.com/task/5453939128139776?logs=ci#L1086
* https://cirrus-ci.com/task/5876151593205760?logs=ci#L1144
* https://cirrus-ci.com/task/5022920222703616?logs=ci#L1246
* https://cirrus-ci.com/task/6579839034982400?logs=ci#L935
π¬ pablomartin4btc commented on pull request "wallet, refactor: Remove Legacy check and error":
(https://github.com/bitcoin/bitcoin/pull/33082#issuecomment-3132733033)
> 131/145 Test # 7: bench_sanity_check ...................Subprocess aborted***Exception: 30.55 sec
> wallet/wallet.cpp:521 UpgradeDescriptorCache: Assertion `IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)' failed.
Yeah, I'm on it, that's why I put it on draft yesterday. Thanks!
(https://github.com/bitcoin/bitcoin/pull/33082#issuecomment-3132733033)
> 131/145 Test # 7: bench_sanity_check ...................Subprocess aborted***Exception: 30.55 sec
> wallet/wallet.cpp:521 UpgradeDescriptorCache: Assertion `IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)' failed.
Yeah, I'm on it, that's why I put it on draft yesterday. Thanks!
π€ jonatack reviewed a pull request: "doc: add note for watch-only wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32866#pullrequestreview-3067803558)
ACK 5888b4a2a5566c64141b78a0e7660a166ec99775
(fwiw, it's unclear to me in this document why sometimes we write "Descriptor" and other times "descriptor")
(https://github.com/bitcoin/bitcoin/pull/32866#pullrequestreview-3067803558)
ACK 5888b4a2a5566c64141b78a0e7660a166ec99775
(fwiw, it's unclear to me in this document why sometimes we write "Descriptor" and other times "descriptor")
π¬ purpleKarrot commented on pull request "kernel: create monolithic kernel static library":
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3132823034)
> is there something wrong with providing and using .pc files in the case where there is a consistent toolchain and nothing is relocated?
Of course not. But how do you guarantee that case? You mentioned it: By building from source, which is exactly my point.
**We cannot provide packages that contain static libraries and/or `.pc` files.**
@theuni said:
> We've always tried to discourage distros and 3rd parties from packaging, in favor of our deterministic build process.
> In Core's
...
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3132823034)
> is there something wrong with providing and using .pc files in the case where there is a consistent toolchain and nothing is relocated?
Of course not. But how do you guarantee that case? You mentioned it: By building from source, which is exactly my point.
**We cannot provide packages that contain static libraries and/or `.pc` files.**
@theuni said:
> We've always tried to discourage distros and 3rd parties from packaging, in favor of our deterministic build process.
> In Core's
...
π¬ maflcko commented on pull request "assumevalid: log every script validation state change":
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2240070540)
you don't need to include those. A header itself does not have any transactions to verify
(https://github.com/bitcoin/bitcoin/pull/32975#discussion_r2240070540)
you don't need to include those. A header itself does not have any transactions to verify
π darosior approved a pull request: "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts"
(https://github.com/bitcoin/bitcoin/pull/32473#pullrequestreview-3067897201)
ACK fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c
(https://github.com/bitcoin/bitcoin/pull/32473#pullrequestreview-3067897201)
ACK fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c
π¬ delta1 commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3132877399)
> should probably lower all three settings, not just the one. However, this breaks a dozen tests or so, there seem to be some more missing that it should be breaking, and feerate estimation currently doesnβt do below 1 s/vB
@murchandamus I'm busy with this to make some progress while @RobinLinus is away, what's the process - create a new PR as a continuation of this one?
When you say "there seem to be some more missing that it *should* be breaking" - what do you mean?
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3132877399)
> should probably lower all three settings, not just the one. However, this breaks a dozen tests or so, there seem to be some more missing that it should be breaking, and feerate estimation currently doesnβt do below 1 s/vB
@murchandamus I'm busy with this to make some progress while @RobinLinus is away, what's the process - create a new PR as a continuation of this one?
When you say "there seem to be some more missing that it *should* be breaking" - what do you mean?
π¬ willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3132892840)
I pushed a commit to make a cache save on pull requests, and then a force push dropping that commit. This worked, but I do see some unexpected ccache hit rates... not sure if anyone here might have an idea about it; I'm at a bit of loss about what could be going wrong here to be honest.
In e.g. the `previous releases` job, the cache was saved here: https://github.com/bitcoin/bitcoin/actions/runs/16589926678/job/46922852038?pr=32989#step:9:77
> Sent 426321843 of 426321843 (100.0%), 81.3 MBs
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3132892840)
I pushed a commit to make a cache save on pull requests, and then a force push dropping that commit. This worked, but I do see some unexpected ccache hit rates... not sure if anyone here might have an idea about it; I'm at a bit of loss about what could be going wrong here to be honest.
In e.g. the `previous releases` job, the cache was saved here: https://github.com/bitcoin/bitcoin/actions/runs/16589926678/job/46922852038?pr=32989#step:9:77
> Sent 426321843 of 426321843 (100.0%), 81.3 MBs
...
π dergoegge approved a pull request: "ci: limit max stack size to 512 KiB"
(https://github.com/bitcoin/bitcoin/pull/33079#pullrequestreview-3067953326)
utACK 3b23f95e3463bf0ab5e293654bb362d07330ff9a
Limiting the stack size in the fuzz job would also be nice but can be left for a follow up.
(https://github.com/bitcoin/bitcoin/pull/33079#pullrequestreview-3067953326)
utACK 3b23f95e3463bf0ab5e293654bb362d07330ff9a
Limiting the stack size in the fuzz job would also be nice but can be left for a follow up.
π pinheadmz approved a pull request: "index: initial sync speedup, parallelize process"
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-3067959910)
re-ACK 9bba5ef0cc5b6890eba2be3a6ed429c4fea5a28f
Changes since last review are minimal responses to my own review suggestions. Built on macos/arm64 and debian/x86. Ran functional and unit tests, ran with `-indexworkers=16` on mainnet fullnode with >900000 blocks
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 9bba5ef0cc5b6890eba2be3a6ed429c4fea5a28f
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmiI39
...
(https://github.com/bitcoin/bitcoin/pull/26966#pullrequestreview-3067959910)
re-ACK 9bba5ef0cc5b6890eba2be3a6ed429c4fea5a28f
Changes since last review are minimal responses to my own review suggestions. Built on macos/arm64 and debian/x86. Ran functional and unit tests, ran with `-indexworkers=16` on mainnet fullnode with >900000 blocks
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 9bba5ef0cc5b6890eba2be3a6ed429c4fea5a28f
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmiI39
...
π¬ pinheadmz commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2240120761)
This is bike shedding now so ok to ignore -- but when we set thread names on Linux they are truncated to 15 characters. So I dunno I guess "thread" and "worker" are redundant in the name of a thread?
https://github.com/bitcoin/bitcoin/blob/2f410ad78c767e37f083d03114f6661b73647af3/src/util/threadnames.cpp#L22-L27
<img width="692" height="317" alt="image" src="https://github.com/user-attachments/assets/8819caee-2731-462c-a914-c595a8599000" />
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2240120761)
This is bike shedding now so ok to ignore -- but when we set thread names on Linux they are truncated to 15 characters. So I dunno I guess "thread" and "worker" are redundant in the name of a thread?
https://github.com/bitcoin/bitcoin/blob/2f410ad78c767e37f083d03114f6661b73647af3/src/util/threadnames.cpp#L22-L27
<img width="692" height="317" alt="image" src="https://github.com/user-attachments/assets/8819caee-2731-462c-a914-c595a8599000" />
π¬ maflcko commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3132951884)
> I'm not sure exactly what is causing the misses here; I would expect this to be 100% (or 99.x%) which I have seen previously.
ccache does not work when the code changes, so you'll have to make sure to compile the same code (usually commit id). Ref:
* https://github.com/bitcoin/bitcoin/actions/runs/16589926678/job/46922852038?pr=32989#step:2:89 [e115ce1e8089037e2bde27600fb092425b0c461d] vs
* https://github.com/bitcoin/bitcoin/actions/runs/16595726917/job/46941763412?pr=32989#step:2:89 [a
...
(https://github.com/bitcoin/bitcoin/pull/32989#issuecomment-3132951884)
> I'm not sure exactly what is causing the misses here; I would expect this to be 100% (or 99.x%) which I have seen previously.
ccache does not work when the code changes, so you'll have to make sure to compile the same code (usually commit id). Ref:
* https://github.com/bitcoin/bitcoin/actions/runs/16589926678/job/46922852038?pr=32989#step:2:89 [e115ce1e8089037e2bde27600fb092425b0c461d] vs
* https://github.com/bitcoin/bitcoin/actions/runs/16595726917/job/46941763412?pr=32989#step:2:89 [a
...
π¬ 1440000bytes commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3132966314)
> Since similar arguments keep coming up: The sole purpose of the minimum relay fee should be DoS protection. Trying to fix the security budget by imposing administered prices on block space seems misguidedβmarket forces will likely circumvent such price controls and make things worse in the process.
This does make it easier for an attacker to use bitcoin p2p as messaging layer. Malwares already use [DNS TXT records](https://dti.domaintools.com/malware-in-dns/) for it although bitcoin p2p wou
...
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3132966314)
> Since similar arguments keep coming up: The sole purpose of the minimum relay fee should be DoS protection. Trying to fix the security budget by imposing administered prices on block space seems misguidedβmarket forces will likely circumvent such price controls and make things worse in the process.
This does make it easier for an attacker to use bitcoin p2p as messaging layer. Malwares already use [DNS TXT records](https://dti.domaintools.com/malware-in-dns/) for it although bitcoin p2p wou
...
π¬ TheCharlatan commented on pull request "kernel: create monolithic kernel static library":
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3132971911)
> But if we want to make sure that 3rd parties use a deterministically built kernel, we either need to force them to use a dynamic library or to build their application inside the same deterministic build environment.
I don't think we should force anything. If we try to, people will find worse ways to get around this limitation, e.g. by manually picking source files out of the repository and creating the library that way (which they did for libbitcoinconsensus). That said, releasing a shared
...
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3132971911)
> But if we want to make sure that 3rd parties use a deterministically built kernel, we either need to force them to use a dynamic library or to build their application inside the same deterministic build environment.
I don't think we should force anything. If we try to, people will find worse ways to get around this limitation, e.g. by manually picking source files out of the repository and creating the library that way (which they did for libbitcoinconsensus). That said, releasing a shared
...
π¬ ryanofsky commented on pull request "kernel: create monolithic kernel static library":
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3132978479)
> **We cannot provide packages that contain static libraries and/or `.pc` files.**
Oh, that makes sense to me. I thought you might have been objecting to having code in the build system that generates .pc files. But I agree distributing .pc files with precompiled static libraries does not seem like it would be very useful, and I assume most developers would just compile from source. Would be interested to know more if this is incorrect and anyone is asking for static binaries or would find th
...
(https://github.com/bitcoin/bitcoin/pull/33077#issuecomment-3132978479)
> **We cannot provide packages that contain static libraries and/or `.pc` files.**
Oh, that makes sense to me. I thought you might have been objecting to having code in the build system that generates .pc files. But I agree distributing .pc files with precompiled static libraries does not seem like it would be very useful, and I assume most developers would just compile from source. Would be interested to know more if this is incorrect and anyone is asking for static binaries or would find th
...