💬 m3dwards commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#issuecomment-2137681414)
Question: Do we want to keep this job whole or split parts out (such as just running the USDT tests)?
If we are keeping it whole as it is with ASAN and LSAN etc, I will investigate if we can get the IPV6 tests working. If I can't get them working, I would like to update the python IPV6 check as it's currently passing even though IPV6 isn't working on the runner.
(https://github.com/bitcoin/bitcoin/pull/30193#issuecomment-2137681414)
Question: Do we want to keep this job whole or split parts out (such as just running the USDT tests)?
If we are keeping it whole as it is with ASAN and LSAN etc, I will investigate if we can get the IPV6 tests working. If I can't get them working, I would like to update the python IPV6 check as it's currently passing even though IPV6 isn't working on the runner.
💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1619089090)
Your code here doesn't use `B` anywhere, so I suspect the comments don't match the code.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1619089090)
Your code here doesn't use `B` anywhere, so I suspect the comments don't match the code.
💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#issuecomment-2137732304)
Added additional `Assume()`s to prevent `operator++` on iterators past the end:
```diff
--- a/src/util/bitset.h
+++ b/src/util/bitset.h
@@ -101,6 +101,7 @@ class IntBitSet
/** Progress to the next 1 bit (only if != IteratorEnd). */
constexpr Iterator& operator++() noexcept
{
+ Assume(m_val != 0);
m_val &= m_val - I{1U};
if (m_val != 0) m_pos = std::countr_zero(m_val);
return *this;
@@ -272,6 +273,7 @@ clas
...
(https://github.com/bitcoin/bitcoin/pull/30160#issuecomment-2137732304)
Added additional `Assume()`s to prevent `operator++` on iterators past the end:
```diff
--- a/src/util/bitset.h
+++ b/src/util/bitset.h
@@ -101,6 +101,7 @@ class IntBitSet
/** Progress to the next 1 bit (only if != IteratorEnd). */
constexpr Iterator& operator++() noexcept
{
+ Assume(m_val != 0);
m_val &= m_val - I{1U};
if (m_val != 0) m_pos = std::countr_zero(m_val);
return *this;
@@ -272,6 +273,7 @@ clas
...
💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1619127144)
I see, then defining `recv_result` as `int64_t` instead of `ssize_t` should be ok on all platforms?
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1619127144)
I see, then defining `recv_result` as `int64_t` instead of `ssize_t` should be ok on all platforms?
💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2137755950)
I missed that before, but the improved text on the BIP made me realize. It looks like an attacker could jack up the difficulty by mining a few difficulty periods with an ASIC and then stop after the last block in a difficulty period. The network would then be on a difficulty some 4<sup>n</sup> higher than before, and stuck looking to mine a first block at full difficulty.
I previously understood that for the difficulty it just goes back to the _latest_ block that has a non-1 difficulty, and d
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2137755950)
I missed that before, but the improved text on the BIP made me realize. It looks like an attacker could jack up the difficulty by mining a few difficulty periods with an ASIC and then stop after the last block in a difficulty period. The network would then be on a difficulty some 4<sup>n</sup> higher than before, and stuck looking to mine a first block at full difficulty.
I previously understood that for the difficulty it just goes back to the _latest_ block that has a non-1 difficulty, and d
...
🤔 TheCharlatan reviewed a pull request: "refactor: use recommended type hiding on multi_index types"
(https://github.com/bitcoin/bitcoin/pull/30194#pullrequestreview-2085822425)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30194#pullrequestreview-2085822425)
Concept ACK
💬 jsarenik commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2137768664)
Tested ACK 86fea43762
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2137768664)
Tested ACK 86fea43762
👋 marcofleon's pull request is ready for review: "fuzz: increase `txorphan` harness stability"
(https://github.com/bitcoin/bitcoin/pull/30186)
(https://github.com/bitcoin/bitcoin/pull/30186)
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1619152037)
@vasild I haven't had a chance to thoroughly review the second commit. Making a separate PR could help.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1619152037)
@vasild I haven't had a chance to thoroughly review the second commit. Making a separate PR could help.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2137789019)
@maflcko That makes absolutely no sense. The shift value is `bits - bitbuf_size`, and it's inside a branch on conditional `bits > bitbuf_size`...?!
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2137789019)
@maflcko That makes absolutely no sense. The shift value is `bits - bitbuf_size`, and it's inside a branch on conditional `bits > bitbuf_size`...?!
💬 Sjors commented on issue "Pause IBD during AssumeUTXO snapshot load":
(https://github.com/bitcoin/bitcoin/issues/29993#issuecomment-2137791010)
IIUC taking `cs_main` blocks all RPC requests and the GUI, and this process can take half an hour or so on a modernish laptop. So I would prefer a less overkill method of pausing IBD. During my own testing I just turned the network off, but that's maybe not ideal either.
(https://github.com/bitcoin/bitcoin/issues/29993#issuecomment-2137791010)
IIUC taking `cs_main` blocks all RPC requests and the GUI, and this process can take half an hour or so on a modernish laptop. So I would prefer a less overkill method of pausing IBD. During my own testing I just turned the network off, but that's maybe not ideal either.
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2137795688)
> I missed that before, but the improved text on the BIP made me realize. It looks like an attacker could jack up the difficulty by mining a few difficulty periods with an ASIC and then stop after the last block in a difficulty period. The network would then be on a difficulty some 4n higher than before, and stuck looking to mine a first block at full difficulty.
>
> I previously understood that for the difficulty it just goes back to the _latest_ block that has a non-1 difficulty, and didn’t
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2137795688)
> I missed that before, but the improved text on the BIP made me realize. It looks like an attacker could jack up the difficulty by mining a few difficulty periods with an ASIC and then stop after the last block in a difficulty period. The network would then be on a difficulty some 4n higher than before, and stuck looking to mine a first block at full difficulty.
>
> I previously understood that for the difficulty it just goes back to the _latest_ block that has a non-1 difficulty, and didn’t
...
🤔 theuni reviewed a pull request: "build: remove `--enable-lcov-branch-coverage`"
(https://github.com/bitcoin/bitcoin/pull/30192#pullrequestreview-2085864110)
I believe this only works because you've exported `LCOV_OPTS` and it makes it into the `make` env?
If you use `./configure LCOV_OPTS="--rc branch_coverage=1"` as you seem to be recommending:
> branch coverage can be enabled by setting `LCOV_OPTS=`... when configuring
I don't think it'd work.
That could be fixed with a plain `AC_SUBST(LCOV_OPTS)`, which would also document it.
(https://github.com/bitcoin/bitcoin/pull/30192#pullrequestreview-2085864110)
I believe this only works because you've exported `LCOV_OPTS` and it makes it into the `make` env?
If you use `./configure LCOV_OPTS="--rc branch_coverage=1"` as you seem to be recommending:
> branch coverage can be enabled by setting `LCOV_OPTS=`... when configuring
I don't think it'd work.
That could be fixed with a plain `AC_SUBST(LCOV_OPTS)`, which would also document it.
💬 fanquake commented on pull request "refactor: use recommended type hiding on multi_index types":
(https://github.com/bitcoin/bitcoin/pull/30194#issuecomment-2137797325)
> This significantly reduces the size of the symbol name lengths that end up in the binaries
About 23'000 bytes worth.
(https://github.com/bitcoin/bitcoin/pull/30194#issuecomment-2137797325)
> This significantly reduces the size of the symbol name lengths that end up in the binaries
About 23'000 bytes worth.
💬 fanquake commented on pull request "build: remove `--enable-lcov-branch-coverage`":
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2137836349)
Right. I started this way, and then think I swapped to exporting because it was less annoying. Might actually just update the docs I'm adding, unless you have a preference?
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2137836349)
Right. I started this way, and then think I swapped to exporting because it was less annoying. Might actually just update the docs I'm adding, unless you have a preference?
💬 theuni commented on pull request "build: remove `--enable-lcov-branch-coverage`":
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2137847021)
I don't like the idea of recommending undocumented vars, so I'd prefer the AC_SUBST unless you're opposed.
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2137847021)
I don't like the idea of recommending undocumented vars, so I'd prefer the AC_SUBST unless you're opposed.
💬 fanquake commented on pull request "build: remove `--enable-lcov-branch-coverage`":
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2137867792)
Ah yea, we won't get it in help. I'll update this tomorrow.
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2137867792)
Ah yea, we won't get it in help. I'll update this tomorrow.
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1619214183)
Reordered the includes.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1619214183)
Reordered the includes.
💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2137880687)
Thanks for the review, i've also rebased on top of master for fresh CI.
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-2137880687)
Thanks for the review, i've also rebased on top of master for fresh CI.
💬 theStack commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1619222418)
Basic question: is this commit (848c4e55da85ec9776ce1c16ca51ea370502125b, "PackageV3Checks: Relax assumptions") strictly needed for this PR or already preparation for future work? The tests pass even if the `Assume`s are kept, and I haven't been able to create a scenario where this condition is true, as the scenario described in the commit message:
```
Consider:
TxA (in mempool) <- TxB (in mempool)
TxA (in mempool) <- TxB' (in package, conflicts with TxB) <- TxC (in package)
```
isn'
...
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1619222418)
Basic question: is this commit (848c4e55da85ec9776ce1c16ca51ea370502125b, "PackageV3Checks: Relax assumptions") strictly needed for this PR or already preparation for future work? The tests pass even if the `Assume`s are kept, and I haven't been able to create a scenario where this condition is true, as the scenario described in the commit message:
```
Consider:
TxA (in mempool) <- TxB (in mempool)
TxA (in mempool) <- TxB' (in package, conflicts with TxB) <- TxC (in package)
```
isn'
...