📝 TheCharlatan opened a pull request: "init: Some small chainstate load improvements"
(https://github.com/bitcoin/bitcoin/pull/31046)
These are mostly followups from #30968, making the code, log lines, error messages, and comments more consistent.
The last commit is an attempt at improving the error reporting when loading the chainstate. It aims to more cleanly distinguish between errors arising from a specific database, and errors where the culprit may be less clear.
(https://github.com/bitcoin/bitcoin/pull/31046)
These are mostly followups from #30968, making the code, log lines, error messages, and comments more consistent.
The last commit is an attempt at improving the error reporting when loading the chainstate. It aims to more cleanly distinguish between errors arising from a specific database, and errors where the culprit may be less clear.
💬 dergoegge commented on pull request "net: fuzz: bypass network magic and checksum validation":
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2396584951)
Copy and pasting the command from the docs doesn't work:
```
$ git apply << "EOF" ...
error: corrupt patch at line 15
```
(https://github.com/bitcoin/bitcoin/pull/31012#issuecomment-2396584951)
Copy and pasting the command from the docs doesn't work:
```
$ git apply << "EOF" ...
error: corrupt patch at line 15
```
⚠️ fanquake opened an issue: "build: RFC Coverage build type"
(https://github.com/bitcoin/bitcoin/issues/31047)
Porting this issue from: https://github.com/hebasto/bitcoin/issues/341.
It's not clear if using our Coverage build type works with Clang, or not. In the linked thread there are simultaneous claims that it "works", but also that it does not work. It seems from the discussion that even using GCC with the Coverage build type is flaky.
There's been suggestions to add a Coverage mode for Clang: https://github.com/hebasto/bitcoin/pull/233, however adding a second way of doing things, when the cu
...
(https://github.com/bitcoin/bitcoin/issues/31047)
Porting this issue from: https://github.com/hebasto/bitcoin/issues/341.
It's not clear if using our Coverage build type works with Clang, or not. In the linked thread there are simultaneous claims that it "works", but also that it does not work. It seems from the discussion that even using GCC with the Coverage build type is flaky.
There's been suggestions to add a Coverage mode for Clang: https://github.com/hebasto/bitcoin/pull/233, however adding a second way of doing things, when the cu
...
💬 josibake commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2396604314)
crACK https://github.com/bitcoin/bitcoin/commit/60ade81516d42d275c1143f49f47e142d32c45fc
Verified that the change is mostly move-only. Makes the code much more clear and cleanly separates initialising / opening the database from loading the database contents into the in memory cache.
> The change was initially motivated to make it easier for users of the kernel library to instantiate a BlockManager that may be used to read data from disk without loading the block index into a cache.
Wil
...
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2396604314)
crACK https://github.com/bitcoin/bitcoin/commit/60ade81516d42d275c1143f49f47e142d32c45fc
Verified that the change is mostly move-only. Makes the code much more clear and cleanly separates initialising / opening the database from loading the database contents into the in memory cache.
> The change was initially motivated to make it easier for users of the kernel library to instantiate a BlockManager that may be used to read data from disk without loading the block index into a cache.
Wil
...
🤔 maflcko reviewed a pull request: "test: use context managers and add file existence checks in feature_fee_estimation.py"
(https://github.com/bitcoin/bitcoin/pull/31030#pullrequestreview-2351664102)
You'll have to rebase, if you are still working on this.
(https://github.com/bitcoin/bitcoin/pull/31030#pullrequestreview-2351664102)
You'll have to rebase, if you are still working on this.
💬 maflcko commented on pull request "test: use context managers and add file existence checks in feature_fee_estimation.py":
(https://github.com/bitcoin/bitcoin/pull/31030#discussion_r1790034935)
This change is in the wrong direction and seems unrelated? pathlib should be used going forward
(https://github.com/bitcoin/bitcoin/pull/31030#discussion_r1790034935)
This change is in the wrong direction and seems unrelated? pathlib should be used going forward
💬 tdb3 commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#discussion_r1790043575)
Yeah, if we can, that would be simpler/preference.
Would need the overhead (message sending, processing, orphanage addition, etc.) to always take less than a second, right? Otherwise there might be intermittent test failure.
If it's always under a second, then maybe something like `setmocktime(int(time.time()))` to force time to start at the beginning of a second might work?
Locally, I very roughly checked runtime and it was consistently under 105ms (not sure how accurate logger is tho
...
(https://github.com/bitcoin/bitcoin/pull/31043#discussion_r1790043575)
Yeah, if we can, that would be simpler/preference.
Would need the overhead (message sending, processing, orphanage addition, etc.) to always take less than a second, right? Otherwise there might be intermittent test failure.
If it's always under a second, then maybe something like `setmocktime(int(time.time()))` to force time to start at the beginning of a second might work?
Locally, I very roughly checked runtime and it was consistently under 105ms (not sure how accurate logger is tho
...
💬 dergoegge commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396661558)
I haven't tried after the transition to CMake but I always had trouble with the build in coverage build mode (e.g. `make cov`). Usually there was problem with path prefixes or a `lcov` version mismatch. The coverage reports themselves were always hard to read and I could not find any documentation on how to read them, for example:
How is branch coverage supposed to be int
...
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396661558)
I haven't tried after the transition to CMake but I always had trouble with the build in coverage build mode (e.g. `make cov`). Usually there was problem with path prefixes or a `lcov` version mismatch. The coverage reports themselves were always hard to read and I could not find any documentation on how to read them, for example:
How is branch coverage supposed to be int
...
💬 vasild commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396670484)
For me it seems best to use each compiler's native workflow. That is: gcov+lcov if using GCC and the [source based coverage](https://github.com/hebasto/bitcoin/issues/341#issuecomment-2316704163) if using Clang.
Would be nice if that is integrated into the build system so it boils down to just `cmake --i-want-coverage`, but if not, then some good instructions in `doc/` would be fine as well.
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396670484)
For me it seems best to use each compiler's native workflow. That is: gcov+lcov if using GCC and the [source based coverage](https://github.com/hebasto/bitcoin/issues/341#issuecomment-2316704163) if using Clang.
Would be nice if that is integrated into the build system so it boils down to just `cmake --i-want-coverage`, but if not, then some good instructions in `doc/` would be fine as well.
💬 maflcko commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#discussion_r1790049818)
I think with mocktime, you don't have to do benchmarks. In most cases it should work on any system, or not at all.
Mocktime applies to most times, so there is no requirement that something happens in less than 105ms.
You should be able to test this by adding a `time.sleep(1.1)` and observing that the test still passes.
(https://github.com/bitcoin/bitcoin/pull/31043#discussion_r1790049818)
I think with mocktime, you don't have to do benchmarks. In most cases it should work on any system, or not at all.
Mocktime applies to most times, so there is no requirement that something happens in less than 105ms.
You should be able to test this by adding a `time.sleep(1.1)` and observing that the test still passes.
💬 fanquake commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396679207)
> For me it seems best to use each compiler's native workflow.
Sure, anyone can still use whatever they like. However our build system doesn't need to (and shouldn't have to) handle all possible combinations of compiler + tooling. Providing a single convenience build type that is the same as what is used by the majority of the developers seems fine, however I don't think we need to natively support anything outside of that, given it's unlikely to be tested/used, or work properly, and just fu
...
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2396679207)
> For me it seems best to use each compiler's native workflow.
Sure, anyone can still use whatever they like. However our build system doesn't need to (and shouldn't have to) handle all possible combinations of compiler + tooling. Providing a single convenience build type that is the same as what is used by the majority of the developers seems fine, however I don't think we need to natively support anything outside of that, given it's unlikely to be tested/used, or work properly, and just fu
...
💬 vasild commented on issue "Listen on random port by default (not 8333)":
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2396705062)
Alright, looks like getting the DNS seeds to propagate ports would be really nice to have or an outright blocker for this.
Currently, about 7% of my IPv4/IPv6 addrman entries use non 8333 port.
```sh
$ bitcoin-cli getnodeaddresses 0 | jq -r 'map(select((.network == "ipv6" or .network == "ipv4") and .port != 8333)) | length'
$ bitcoin-cli getnodeaddresses 0 | jq -r 'map(select(.network == "ipv6" or .network == "ipv4")) | length'
```
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2396705062)
Alright, looks like getting the DNS seeds to propagate ports would be really nice to have or an outright blocker for this.
Currently, about 7% of my IPv4/IPv6 addrman entries use non 8333 port.
```sh
$ bitcoin-cli getnodeaddresses 0 | jq -r 'map(select((.network == "ipv6" or .network == "ipv4") and .port != 8333)) | length'
$ bitcoin-cli getnodeaddresses 0 | jq -r 'map(select(.network == "ipv6" or .network == "ipv4")) | length'
```
💬 tdb3 commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#discussion_r1790087981)
Thanks.
The following passed. Will push.
```diff
diff --git a/test/functional/rpc_orphans.py b/test/functional/rpc_orphans.py
index 207647f548..825657a1ed 100755
--- a/test/functional/rpc_orphans.py
+++ b/test/functional/rpc_orphans.py
@@ -96,11 +96,15 @@ class OrphanRPCsTest(BitcoinTestFramework):
tx_child_2 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_2["new_utxo"])
peer_1 = node.add_p2p_connection(P2PInterface())
peer_2 = node.add_p2p_co
...
(https://github.com/bitcoin/bitcoin/pull/31043#discussion_r1790087981)
Thanks.
The following passed. Will push.
```diff
diff --git a/test/functional/rpc_orphans.py b/test/functional/rpc_orphans.py
index 207647f548..825657a1ed 100755
--- a/test/functional/rpc_orphans.py
+++ b/test/functional/rpc_orphans.py
@@ -96,11 +96,15 @@ class OrphanRPCsTest(BitcoinTestFramework):
tx_child_2 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_2["new_utxo"])
peer_1 = node.add_p2p_connection(P2PInterface())
peer_2 = node.add_p2p_co
...
💬 tdb3 commented on pull request "rpc: getorphantxs follow-up":
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2396730642)
Pushed to ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9 to replace `assert_approx` with `assert_equal` (using `setmocktime`)
(https://github.com/bitcoin/bitcoin/pull/31043#issuecomment-2396730642)
Pushed to ae17f9050e0c2a6fd31cd9f5d2f3ad8066f02cc9 to replace `assert_approx` with `assert_equal` (using `setmocktime`)
👍 tdb3 approved a pull request: "ci: set a ctest test timeout of 1200 (20 minutes)"
(https://github.com/bitcoin/bitcoin/pull/31026#pullrequestreview-2351756890)
re ACK 56aad83307e46983a397236bd0959e634207f83e
(https://github.com/bitcoin/bitcoin/pull/31026#pullrequestreview-2351756890)
re ACK 56aad83307e46983a397236bd0959e634207f83e
💬 maflcko commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#discussion_r1790111622)
Any reason to filter to only mempool_?
(https://github.com/bitcoin/bitcoin/pull/30956#discussion_r1790111622)
Any reason to filter to only mempool_?
💬 maflcko commented on issue "ci: ConnectionRefusedError: [WinError 10061] No connection could be made because the target machine actively refused it":
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2396787199)
https://github.com/bitcoin/bitcoin/actions/runs/11164991068/job/31035622913?pr=30982#step:12:242
(https://github.com/bitcoin/bitcoin/issues/30390#issuecomment-2396787199)
https://github.com/bitcoin/bitcoin/actions/runs/11164991068/job/31035622913?pr=30982#step:12:242
💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396818224)
It's nice to keep macOS 11 support, but I don't have a (easy) way to test it, so I wouldn't put too much effort in it.
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2396818224)
It's nice to keep macOS 11 support, but I don't have a (easy) way to test it, so I wouldn't put too much effort in it.
💬 maflcko commented on pull request "Add -pausebackgroundsync startup option":
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2396823190)
I tend to agree with https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391695605. Adding a test-only option in this way won't be useful to benchmark previous releases (and compare them), and seems more work than just using a test-only datadir dump.
> The main use case I have in mind is some who doesn't want to do background validation.
There are reasons to do the background validation, see https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-2022485737 . If you are adding a
...
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2396823190)
I tend to agree with https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391695605. Adding a test-only option in this way won't be useful to benchmark previous releases (and compare them), and seems more work than just using a test-only datadir dump.
> The main use case I have in mind is some who doesn't want to do background validation.
There are reasons to do the background validation, see https://github.com/bitcoin/bitcoin/pull/28616#issuecomment-2022485737 . If you are adding a
...
🚀 fanquake merged a pull request: "ci: set a ctest test timeout of 1200 (20 minutes)"
(https://github.com/bitcoin/bitcoin/pull/31026)
(https://github.com/bitcoin/bitcoin/pull/31026)