💬 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'
...
💬 murchandamus commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#issuecomment-2137911720)
> Happy to reack, but you need to fix clang-tidy first
Fixed the issue with `clang-tidy`, sorry about that
(https://github.com/bitcoin/bitcoin/pull/28366#issuecomment-2137911720)
> Happy to reack, but you need to fix clang-tidy first
Fixed the issue with `clang-tidy`, sorry about that
💬 chrisguida commented on pull request "validation: sync chainstate to disk after syncing to tip":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1619240097)
Can we clarify this message a bit to emphasize that this rescheduling does not preclude periodic flushes to disk?
It's slightly confusing to see this message as it appears that disk flushes are being deferred indefinitely.
See: https://github.com/bitcoinknots/bitcoin/issues/70#issuecomment-2136078696
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1619240097)
Can we clarify this message a bit to emphasize that this rescheduling does not preclude periodic flushes to disk?
It's slightly confusing to see this message as it appears that disk flushes are being deferred indefinitely.
See: https://github.com/bitcoinknots/bitcoin/issues/70#issuecomment-2136078696
💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1619252262)
It is duplicating part of that, given `CConnman::m_max_automatic_outbound` is private.
I agree it is adding more complexity, but not accounting for it in the minimum means that could be in a situation where no single outbound connection is accounted for, so the user is basically only connected to manuals.
I acknowledge that's fairly rare, but the two reasons why we are currently not facing this issue is, first, because the minimum is fairly low, so it would need a system with really limite
...
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1619252262)
It is duplicating part of that, given `CConnman::m_max_automatic_outbound` is private.
I agree it is adding more complexity, but not accounting for it in the minimum means that could be in a situation where no single outbound connection is accounted for, so the user is basically only connected to manuals.
I acknowledge that's fairly rare, but the two reasons why we are currently not facing this issue is, first, because the minimum is fairly low, so it would need a system with really limite
...
💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2137944959)
> Yes, this has also been described [here by AJ](https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2092080764).
I must have read that before it was edited and missed the edit.
> @Sjors suggestion to change `nActualTimespan` was also aimed at this problem but I have a few doubts outlined above. I think @ajtowns suggestion to use the version field is the only complete solution for this problem so far.
Thanks for keeping the overview! :)
I guess if someone were to troll testnet
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2137944959)
> Yes, this has also been described [here by AJ](https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2092080764).
I must have read that before it was edited and missed the edit.
> @Sjors suggestion to change `nActualTimespan` was also aimed at this problem but I have a few doubts outlined above. I think @ajtowns suggestion to use the version field is the only complete solution for this problem so far.
Thanks for keeping the overview! :)
I guess if someone were to troll testnet
...