💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474913651)
```suggestion
# Multiply fee by 5, which should easily cover the cost to replace (but is still too large a cluster). Otherwise, use the target vsize at 10sat/vB
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474913651)
```suggestion
# Multiply fee by 5, which should easily cover the cost to replace (but is still too large a cluster). Otherwise, use the target vsize at 10sat/vB
```
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2475071643)
Since this harness is making blocks, I think it'd be good to have sigops naturally appearing in the constructed blocks. Ran this for a bit, seemed to work?
```
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index 8a5863c5d5..413ab0db4c 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -291,9 +291,18 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
tx_mut.vin.push_back(in);
}
+
+ // Check
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2475071643)
Since this harness is making blocks, I think it'd be good to have sigops naturally appearing in the constructed blocks. Ran this for a bit, seemed to work?
```
diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp
index 8a5863c5d5..413ab0db4c 100644
--- a/src/test/fuzz/tx_pool.cpp
+++ b/src/test/fuzz/tx_pool.cpp
@@ -291,9 +291,18 @@ FUZZ_TARGET(tx_pool_standard, .init = initialize_tx_pool)
tx_mut.vin.push_back(in);
}
+
+ // Check
...
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2475016196)
just extended it to clusters of size three and made log more sensible
```
diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py
index 759e3cb07d..c54534f6f8 100755
--- a/test/functional/mempool_package_rbf.py
+++ b/test/functional/mempool_package_rbf.py
@@ -94,5 +94,5 @@ class PackageRBFTest(BitcoinTestFramework):
def test_package_rbf_basic(self):
- self.log.info("Test that a child can pay to replace its parents' conflicts of cluster
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2475016196)
just extended it to clusters of size three and made log more sensible
```
diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py
index 759e3cb07d..c54534f6f8 100755
--- a/test/functional/mempool_package_rbf.py
+++ b/test/functional/mempool_package_rbf.py
@@ -94,5 +94,5 @@ class PackageRBFTest(BitcoinTestFramework):
def test_package_rbf_basic(self):
- self.log.info("Test that a child can pay to replace its parents' conflicts of cluster
...
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474283513)
no-op trimming makes sense too (probably a smarter of way of having this hit than I suggest here)
```suggestion
tx_pool.TrimToSize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0U, tx_pool.DynamicMemoryUsage() * 2));
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2474283513)
no-op trimming makes sense too (probably a smarter of way of having this hit than I suggest here)
```suggestion
tx_pool.TrimToSize(fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0U, tx_pool.DynamicMemoryUsage() * 2));
```
💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475098091)
OK this was a mistake on my part, @hodlinator's suggested change passes, because segwit node's index is *not* `0` any more, it just got connected so it's `-1`, and any ways, for the test that follows it should not matter if the peer is HB or not, since the peer sends a header and waits for getdata, so this is solicited. Should I add separate test cases for both? For now, I'll just drop this line.
It also seems weird that we'll accept a CMPCTBLOCK from someone that hasn't given us a SENDCMPCT
...
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475098091)
OK this was a mistake on my part, @hodlinator's suggested change passes, because segwit node's index is *not* `0` any more, it just got connected so it's `-1`, and any ways, for the test that follows it should not matter if the peer is HB or not, since the peer sends a header and waits for getdata, so this is solicited. Should I add separate test cases for both? For now, I'll just drop this line.
It also seems weird that we'll accept a CMPCTBLOCK from someone that hasn't given us a SENDCMPCT
...
💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2475135383)
Sorry for being unclear. I don't think there is any broken assumption either.
However, if we made this a non-atomic as suggested, then *if* something in the future were to break this assumption and run these tasks on multiple threads the unit test would break. Thus it would act as a regression test.
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2475135383)
Sorry for being unclear. I don't think there is any broken assumption either.
However, if we made this a non-atomic as suggested, then *if* something in the future were to break this assumption and run these tasks on multiple threads the unit test would break. Thus it would act as a regression test.
💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3463589829)
Guix build for c4ea4210abb20afa3724a2dc1384b0b91d099b31:
3c68ce9be2dfa7c2b20962f119ac48d760e142162da5d340011e496f099ee38c dist-archive/bitcoin-c4ea4210abb2.tar.gz
2ccb113dd38bd2ca5db8dd5ec20540c09f149e03789086b738dc62ab74e6892c x86_64-linux-gnu/bitcoin-c4ea4210abb2-x86_64-linux-gnu-debug.tar.gz
a4aed426dd114dab2ed1f62904eda03805ee0e516c83cebd366d4e7e50387da4 x86_64-linux-gnu/bitcoin-c4ea4210abb2-x86_64-linux-gnu.tar.gz
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3463589829)
Guix build for c4ea4210abb20afa3724a2dc1384b0b91d099b31:
3c68ce9be2dfa7c2b20962f119ac48d760e142162da5d340011e496f099ee38c dist-archive/bitcoin-c4ea4210abb2.tar.gz
2ccb113dd38bd2ca5db8dd5ec20540c09f149e03789086b738dc62ab74e6892c x86_64-linux-gnu/bitcoin-c4ea4210abb2-x86_64-linux-gnu-debug.tar.gz
a4aed426dd114dab2ed1f62904eda03805ee0e516c83cebd366d4e7e50387da4 x86_64-linux-gnu/bitcoin-c4ea4210abb2-x86_64-linux-gnu.tar.gz
💬 hebasto commented on pull request "guix: Use UCRT runtime for Windows release binaries":
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2475155565)
@trevarj
Would it work with the recursion in `make-mingw-w64/implementation`?
(https://github.com/bitcoin/bitcoin/pull/33593#discussion_r2475155565)
@trevarj
Would it work with the recursion in `make-mingw-w64/implementation`?
💬 ismaelsadeeq commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3463610029)
Thanks for your review @ryanofsky I will address the comments soon.
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3463610029)
Thanks for your review @ryanofsky I will address the comments soon.
⚠️ DoctorBuzz1 opened an issue: "Limit "Bulk Dust" with a default filter or consensus.."
(https://github.com/bitcoin/bitcoin/issues/33737)
I’m exploring a potential default filter or consensus-level rule (since a large number of people believe that default filters don't work) to discourage UTXO-bloat patterns without touching Script, witness data, or the block size limit.
The idea is to target “bulk dust” transactions — those that create large numbers of extremely small outputs — which are the main cause of long-term UTXO set growth.
These types of "bulk dust" transactions have been the No. 1 reason cited for wanting to expand th
...
(https://github.com/bitcoin/bitcoin/issues/33737)
I’m exploring a potential default filter or consensus-level rule (since a large number of people believe that default filters don't work) to discourage UTXO-bloat patterns without touching Script, witness data, or the block size limit.
The idea is to target “bulk dust” transactions — those that create large numbers of extremely small outputs — which are the main cause of long-term UTXO set growth.
These types of "bulk dust" transactions have been the No. 1 reason cited for wanting to expand th
...
💬 parabitdev commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463668325)
A few notes that should not be taken as support/detraction for the question of removal (I am opposed to dns seeds in general, but there is not a better alternative currently that I am aware of).
Firstly, the seed in question appears to resolve to a server in Germany instead of the one pointed to by `luke.dashjr.org` (the one that was previously compromised and features a disclaimer about it's safety). As such, I am less concerned by his deviations from standard security practice with that serve
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463668325)
A few notes that should not be taken as support/detraction for the question of removal (I am opposed to dns seeds in general, but there is not a better alternative currently that I am aware of).
Firstly, the seed in question appears to resolve to a server in Germany instead of the one pointed to by `luke.dashjr.org` (the one that was previously compromised and features a disclaimer about it's safety). As such, I am less concerned by his deviations from standard security practice with that serve
...
💬 davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475267452)
Good catch, in the branch I'll push soon I address this by checking both a solicited cmpctblock and unsolicited one.
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475267452)
Good catch, in the branch I'll push soon I address this by checking both a solicited cmpctblock and unsolicited one.
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2475307568)
> Sorry for being unclear. I don't think there is any broken assumption either.
> However, if we made this a non-atomic as suggested, then _if_ something in the future were to break this assumption and run these tasks on multiple threads the unit test would break. Thus it would act as a regression test.
But as you said, the thread sanitizer would likely complain about that. We’re accessing the same variable from different threads (main and worker). The main issue, however, will probably be r
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2475307568)
> Sorry for being unclear. I don't think there is any broken assumption either.
> However, if we made this a non-atomic as suggested, then _if_ something in the future were to break this assumption and run these tasks on multiple threads the unit test would break. Thus it would act as a regression test.
But as you said, the thread sanitizer would likely complain about that. We’re accessing the same variable from different threads (main and worker). The main issue, however, will probably be r
...
💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475344730)
+1 for separate test cases for solicited / unsolicited. I agree the test can be improved in a dedicated PR.
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2475344730)
+1 for separate test cases for solicited / unsolicited. I agree the test can be improved in a dedicated PR.
💬 Psifour commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463882411)
A few notes that should not be taken as support/detraction for the question of removal (I am opposed to dns seeds in general, but there is not a better alternative currently that I am aware of).
Firstly, the seed in question appears to resolve to a server in Germany instead of the one pointed to by `luke.dashjr.org` (the one that was previously compromised and features a disclaimer about it's safety). As such, I am less concerned by his deviations from standard security practice with that speci
...
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3463882411)
A few notes that should not be taken as support/detraction for the question of removal (I am opposed to dns seeds in general, but there is not a better alternative currently that I am aware of).
Firstly, the seed in question appears to resolve to a server in Germany instead of the one pointed to by `luke.dashjr.org` (the one that was previously compromised and features a disclaimer about it's safety). As such, I am less concerned by his deviations from standard security practice with that speci
...
💬 ajtowns commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-3463882765)
> After [#27675](https://github.com/bitcoin/bitcoin/pull/27675) (but also before, with a few more restrictions), we relay any tx from our mempool (except for those that arrived after the last INV sent to the peer), so someone wanting to fingerprint our mempool could just ask for those transactions directly instead I think and then check whether we send it to them or answer with `NOTFOUND`.
I don't think this issue makes sense as an attack -- we aim to avoid giving away tight timing information
...
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-3463882765)
> After [#27675](https://github.com/bitcoin/bitcoin/pull/27675) (but also before, with a few more restrictions), we relay any tx from our mempool (except for those that arrived after the last INV sent to the peer), so someone wanting to fingerprint our mempool could just ask for those transactions directly instead I think and then check whether we send it to them or answer with `NOTFOUND`.
I don't think this issue makes sense as an attack -- we aim to avoid giving away tight timing information
...
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3463894000)
I'd say it's time to update the PR description:
<details>
<summary>24% faster reindex-chainstate | 921129 blocks | dbcache 450 | rpi5-16-2 | aarch64 | Cortex-A76 | 4 cores | 15Gi RAM | ext4 | SSD</summary>
```
COMMITS="cb0fdfdf3704d5ffe6ccc634de6fdba6b7b57a85 2aa510348143521a14146e41b5cf87cb3e60b29e"; \
STOP=921129; DBCACHE=450; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q ori
...
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3463894000)
I'd say it's time to update the PR description:
<details>
<summary>24% faster reindex-chainstate | 921129 blocks | dbcache 450 | rpi5-16-2 | aarch64 | Cortex-A76 | 4 cores | 15Gi RAM | ext4 | SSD</summary>
```
COMMITS="cb0fdfdf3704d5ffe6ccc634de6fdba6b7b57a85 2aa510348143521a14146e41b5cf87cb3e60b29e"; \
STOP=921129; DBCACHE=450; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q ori
...
💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2475412779)
I agree that mentioning a consensus error makes sense, so I added it to the warning message. Note that I didn't change the existing `CheckForkWarningConditions()` message as well, but could do that too if reviewers suggest it.
In principle, I don't see a difference whether this scenario happens during IBD or close to the tip. In both cases, it is most likely corruption but a consensus error is possible too. Current behaviour is that in the IBD case, we will cycle through header-syncs with n
...
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2475412779)
I agree that mentioning a consensus error makes sense, so I added it to the warning message. Note that I didn't change the existing `CheckForkWarningConditions()` message as well, but could do that too if reviewers suggest it.
In principle, I don't see a difference whether this scenario happens during IBD or close to the tip. In both cases, it is most likely corruption but a consensus error is possible too. Current behaviour is that in the IBD case, we will cycle through header-syncs with n
...
💬 TheCharlatan commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2475436253)
In commit fa128d1488c08d9816f425c73d01db4c1597ee68:
Would the return type of `GetTotalSize` fit into the criteria for this refactor?
```diff
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index cf6da55aa9..9d50dfe949 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -195 +195 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
- unsigned int tx_missing_size = 0;
+ uint32_t tx_missing_size = 0;
diff --git a/src/ne
...
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2475436253)
In commit fa128d1488c08d9816f425c73d01db4c1597ee68:
Would the return type of `GetTotalSize` fit into the criteria for this refactor?
```diff
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index cf6da55aa9..9d50dfe949 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -195 +195 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
- unsigned int tx_missing_size = 0;
+ uint32_t tx_missing_size = 0;
diff --git a/src/ne
...
💬 l0rinc commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3463979308)
I have also documented some workarounds for mac in https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-3361478681 - they kinda' work, without sanitizers at least, which cannot handle exceptions for some reason.
But I'm not sure why we want to give up fuzzing on Macs, everything we do is a whack-a-mole game, if we want people to write fuzz tests, we can't expect them to run them on a different machine and not even try them locally. We'll just end up with 100 PR pushes because people will
...
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3463979308)
I have also documented some workarounds for mac in https://github.com/bitcoin/bitcoin/pull/32084#issuecomment-3361478681 - they kinda' work, without sanitizers at least, which cannot handle exceptions for some reason.
But I'm not sure why we want to give up fuzzing on Macs, everything we do is a whack-a-mole game, if we want people to write fuzz tests, we can't expect them to run them on a different machine and not even try them locally. We'll just end up with 100 PR pushes because people will
...