💬 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
...
💬 sr-gi commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1619278941)
This could be a simpler alternative, that accounts for the automatic within the minimum
```diff
// Automatic outbounds are already accounted for in nUserMaxConnections, but we want to make sure they count towards the minimum later on
int auto_outbounds = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS + MAX_BLOCK_RELAY_ONLY_CONNECTIONS + MAX_FEELER_CONNECTIONS, nUserMaxConnections);
- // Reserve enough FDs to account for the bare minimum, plus any manual connections, plus the bound interfa
...
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1619278941)
This could be a simpler alternative, that accounts for the automatic within the minimum
```diff
// Automatic outbounds are already accounted for in nUserMaxConnections, but we want to make sure they count towards the minimum later on
int auto_outbounds = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS + MAX_BLOCK_RELAY_ONLY_CONNECTIONS + MAX_FEELER_CONNECTIONS, nUserMaxConnections);
- // Reserve enough FDs to account for the bare minimum, plus any manual connections, plus the bound interfa
...
💬 brunoerg commented on pull request "rpc: net: follow-ups for #30062":
(https://github.com/bitcoin/bitcoin/pull/30183#discussion_r1619292782)
done
(https://github.com/bitcoin/bitcoin/pull/30183#discussion_r1619292782)
done
💬 brunoerg commented on pull request "rpc: net: follow-ups for #30062":
(https://github.com/bitcoin/bitcoin/pull/30183#issuecomment-2137992066)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30183#discussion_r1618155192.
(https://github.com/bitcoin/bitcoin/pull/30183#issuecomment-2137992066)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/30183#discussion_r1618155192.
💬 luke-jr commented on pull request "rpc: net: follow-ups for #30062":
(https://github.com/bitcoin/bitcoin/pull/30183#issuecomment-2138039768)
Not sure the asmap feature cares if your map is based on BGP or something else?
(https://github.com/bitcoin/bitcoin/pull/30183#issuecomment-2138039768)
Not sure the asmap feature cares if your map is based on BGP or something else?
💬 jb55 commented on issue "add option to bumpfee RPC to prevent it adding new inputs":
(https://github.com/bitcoin/bitcoin/issues/20935#issuecomment-2138042959)
another thing I wanted to do today with a single output is to bump the fee + add an output. from what I can tell there are simply no tools that exist that allow me to do this with a psbt.
(https://github.com/bitcoin/bitcoin/issues/20935#issuecomment-2138042959)
another thing I wanted to do today with a single output is to bump the fee + add an output. from what I can tell there are simply no tools that exist that allow me to do this with a psbt.
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1619345525)
> is this commit (848c4e5, "PackageV3Checks: Relax assumptions") strictly needed for this PR or already preparation for future work?
If it's not removed the next commit will cause failures.
It hits pretty in the fuzzing `package_eval` fuzzing target. I could add a unit test for it, but it doesn't really test much, except that we don't crash right before removing the soon to be erroneous assertion. Here's an input you can run to verify:
```
EQQAAAgAAAAAAAAAAAAAAAUAAAAAAAAAAAAAAAA6AAAACAA
...
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1619345525)
> is this commit (848c4e5, "PackageV3Checks: Relax assumptions") strictly needed for this PR or already preparation for future work?
If it's not removed the next commit will cause failures.
It hits pretty in the fuzzing `package_eval` fuzzing target. I could add a unit test for it, but it doesn't really test much, except that we don't crash right before removing the soon to be erroneous assertion. Here's an input you can run to verify:
```
EQQAAAgAAAAAAAAAAAAAAAUAAAAAAAAAAAAAAAA6AAAACAA
...