💬 maflcko commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1847411927)
> > They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
>
> Similar rationale as in your OP -- it seems confusing to have unreachable code. Happy re-review if you agree otherwise NBD.
Ok, but that'd require touching leveldb, which is a subtree, so I can't touch it here.
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1847411927)
> > They seem harmless, so if the author wants them and reviewers are fine with them, it seems fine?
>
> Similar rationale as in your OP -- it seems confusing to have unreachable code. Happy re-review if you agree otherwise NBD.
Ok, but that'd require touching leveldb, which is a subtree, so I can't touch it here.
💬 maflcko commented on pull request "wallet: batch all individual spkms setup db writes in a single db txn":
(https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1420689322)
> Also, probably for another working path, wouldn't be bad to introduce a way to clean up stuff within the benchmark execution that is deliberately not included in the bench results.
Sgtm, but if the cleanup only takes a very short time, I think it is fine to just leave as-is.
(https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1420689322)
> Also, probably for another working path, wouldn't be bad to introduce a way to clean up stuff within the benchmark execution that is deliberately not included in the bench results.
Sgtm, but if the cleanup only takes a very short time, I think it is fine to just leave as-is.
⚠️ Wolfstorm321 opened an issue: "Software wont launch"
(https://github.com/bitcoin/bitcoin/issues/29033)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Version 0.23.0.0
I ran a full node between 2015 and 2020, pruned since 2020.
I decided to run a full node recently, as I want to import keys to a new wallet.
However, the program wont open while running a full node.
Started synching yesterday at morning, it synched up to 2018.
Closed the program, turned off the computer, went to sleep.
Waked up today, turned on the computer, o
...
(https://github.com/bitcoin/bitcoin/issues/29033)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Version 0.23.0.0
I ran a full node between 2015 and 2020, pruned since 2020.
I decided to run a full node recently, as I want to import keys to a new wallet.
However, the program wont open while running a full node.
Started synching yesterday at morning, it synched up to 2018.
Closed the program, turned off the computer, went to sleep.
Waked up today, turned on the computer, o
...
💬 sdaftuar commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1847460679)
So thinking conceptually about this, I'm most concerned by the new `CheckMinerScores()` criterion, where we only check the replacement package's feerate against the ancestor feerates of the indirect conflicts.
I think there are two issues with this approach:
1) If you look at `GetModFeesWithAncestors()/GetSizeWithAncestors()`, that can be greater than the transaction's individual feerate (ie if the parent of a transaction has a higher feerate than the child you're conflicting with). So this
...
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1847460679)
So thinking conceptually about this, I'm most concerned by the new `CheckMinerScores()` criterion, where we only check the replacement package's feerate against the ancestor feerates of the indirect conflicts.
I think there are two issues with this approach:
1) If you look at `GetModFeesWithAncestors()/GetSizeWithAncestors()`, that can be greater than the transaction's individual feerate (ie if the parent of a transaction has a higher feerate than the child you're conflicting with). So this
...
⚠️ achow101 reopened an issue: "`-min` does not minimize wallet loading dialog"
(https://github.com/bitcoin-core/gui/issues/748)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
A user on the Bitcoin stackexchange [reported](https://bitcoin.stackexchange.com/questions/119283/is-it-possible-to-run-bitcoin-core-gui-at-the-same-time-as-bitcoind) that starting `-min` still results in the `Loading wallets` dialog box appearing.
### Expected behaviour
`-min` should minimize all startup dialogs.
### Steps to reproduce
Start with `-min` with wallets automatically load
...
(https://github.com/bitcoin-core/gui/issues/748)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
A user on the Bitcoin stackexchange [reported](https://bitcoin.stackexchange.com/questions/119283/is-it-possible-to-run-bitcoin-core-gui-at-the-same-time-as-bitcoind) that starting `-min` still results in the `Loading wallets` dialog box appearing.
### Expected behaviour
`-min` should minimize all startup dialogs.
### Steps to reproduce
Start with `-min` with wallets automatically load
...
💬 achow101 commented on issue "`-min` does not minimize wallet loading dialog":
(https://github.com/bitcoin-core/gui/issues/748#issuecomment-1847496436)
A user on IRC says that this was not fixed in 26.0.
(https://github.com/bitcoin-core/gui/issues/748#issuecomment-1847496436)
A user on IRC says that this was not fixed in 26.0.
💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1420774368)
Good idea, will do, probably best not to sneak in something unrelated even if it is small. I'll remove this from this PR. Thanks!
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1420774368)
Good idea, will do, probably best not to sneak in something unrelated even if it is small. I'll remove this from this PR. Thanks!
💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1847520923)
> > Ok, I'll keep poking. But it seems like this approach likely won't work :(
>
> What about hiding the function inside a compilation unit, where they are turned into `bswap`, and then rely on LTO to replace the `call internal_bswap_64` with `bswap`? (Haven't tried this)
Hah, that's fun. Too fun in fact, so I'm not going to try because it would make things even more complicated if it worked :p
Because even if it worked, we couldn't possibly require LTO as a means of achieving basic perfo
...
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1847520923)
> > Ok, I'll keep poking. But it seems like this approach likely won't work :(
>
> What about hiding the function inside a compilation unit, where they are turned into `bswap`, and then rely on LTO to replace the `call internal_bswap_64` with `bswap`? (Haven't tried this)
Hah, that's fun. Too fun in fact, so I'm not going to try because it would make things even more complicated if it worked :p
Because even if it worked, we couldn't possibly require LTO as a means of achieving basic perfo
...
💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1420786252)
Yes, there are essentially 3 changes here:
- using `std::endian`
- using `std::countl_zero`
- removing the non-standard byteswaps
The first two are useful regardless, though obviously much less so without doing the third.
I'll update this to break up the commits.
As an alternative to what's here, I'm going to try switching our byteswaps to compiler builtins for gcc/clang/msvc. That still leaves us with compiler-specific behavior, but that would at least be an improvement over the cur
...
(https://github.com/bitcoin/bitcoin/pull/28674#discussion_r1420786252)
Yes, there are essentially 3 changes here:
- using `std::endian`
- using `std::countl_zero`
- removing the non-standard byteswaps
The first two are useful regardless, though obviously much less so without doing the third.
I'll update this to break up the commits.
As an alternative to what's here, I'm going to try switching our byteswaps to compiler builtins for gcc/clang/msvc. That still leaves us with compiler-specific behavior, but that would at least be an improvement over the cur
...
🤔 pinheadmz reviewed a pull request: "Make bitcoin-tx replaceable value optional"
(https://github.com/bitcoin/bitcoin/pull/29022#pullrequestreview-1772806380)
concept ACK 605a147b3bf93b7e4325afa4a4df61ffb3523012
I think the test failures are because on CI we copy everything we need for testing into a new directory in the container, and the files you added need to be listed as well in Makefile.md under `EXTRA_DIST`.
(https://github.com/bitcoin/bitcoin/pull/29022#pullrequestreview-1772806380)
concept ACK 605a147b3bf93b7e4325afa4a4df61ffb3523012
I think the test failures are because on CI we copy everything we need for testing into a new directory in the container, and the files you added need to be listed as well in Makefile.md under `EXTRA_DIST`.
💬 pinheadmz commented on pull request "Make bitcoin-tx replaceable value optional":
(https://github.com/bitcoin/bitcoin/pull/29022#discussion_r1420776229)
I think you can just remove this last sentence. The command DOES take an action if no inputs are provided (you added a test for this!), and the impact of this option in that case I think is handled already by the first two sentences.
(https://github.com/bitcoin/bitcoin/pull/29022#discussion_r1420776229)
I think you can just remove this last sentence. The command DOES take an action if no inputs are provided (you added a test for this!), and the impact of this option in that case I think is handled already by the first two sentences.
📝 theStack opened a pull request: "test: detect OS in functional tests consistently using `platform.system()`"
(https://github.com/bitcoin/bitcoin/pull/29034)
There are at least three ways to detect the operating system in Python3:
- `os.name` (https://docs.python.org/3.9/library/os.html#os.name)
- `sys.platform` (https://docs.python.org/3.9/library/sys.html#sys.platform)
- `platform.system()` (https://docs.python.org/3.9/library/platform.html#platform.system)
We are currently using all of them in functional tests (both in individual tests and shared test framework code), which seems a bit messy. This PR consolidates into using `platform.system(
...
(https://github.com/bitcoin/bitcoin/pull/29034)
There are at least three ways to detect the operating system in Python3:
- `os.name` (https://docs.python.org/3.9/library/os.html#os.name)
- `sys.platform` (https://docs.python.org/3.9/library/sys.html#sys.platform)
- `platform.system()` (https://docs.python.org/3.9/library/platform.html#platform.system)
We are currently using all of them in functional tests (both in individual tests and shared test framework code), which seems a bit messy. This PR consolidates into using `platform.system(
...
💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1420802246)
We do remove the custom directory at the beginning of the test:
https://github.com/bitcoin/bitcoin/pull/26564/files#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R164
so I think this restriction is still needed. I just verified that `/tmp/test_common_Bitcoin\ Core` can be a symlink, so that would allow you to use any location, right? (I could add that to the README but not sure it's worth it.)
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1420802246)
We do remove the custom directory at the beginning of the test:
https://github.com/bitcoin/bitcoin/pull/26564/files#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R164
so I think this restriction is still needed. I just verified that `/tmp/test_common_Bitcoin\ Core` can be a symlink, so that would allow you to use any location, right? (I could add that to the README but not sure it's worth it.)
📝 theStack opened a pull request: "test: fix `addnode` functional test failure on OpenBSD"
(https://github.com/bitcoin/bitcoin/pull/29035)
This is the functional test counterpart of PR #28891 / commit 007d6f0e85bc329040bb405ef6016339518caa66 (unfortunately, I missed it back then and only ran the unit tests -- sorry for the noise).
master branch on OpenBSD 7.4:
```
$ ./test/functional/rpc_net.py
2023-12-08T17:29:05.057000Z TestFramework (INFO): PRNG seed is: 6024296850131317403
2023-12-08T17:29:05.058000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_au3zchif
2023-12-08T17:29:05.618000Z TestFramew
...
(https://github.com/bitcoin/bitcoin/pull/29035)
This is the functional test counterpart of PR #28891 / commit 007d6f0e85bc329040bb405ef6016339518caa66 (unfortunately, I missed it back then and only ran the unit tests -- sorry for the noise).
master branch on OpenBSD 7.4:
```
$ ./test/functional/rpc_net.py
2023-12-08T17:29:05.057000Z TestFramework (INFO): PRNG seed is: 6024296850131317403
2023-12-08T17:29:05.058000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_au3zchif
2023-12-08T17:29:05.618000Z TestFramew
...
💬 theStack commented on issue "test: Intermittent issue in rpc_net.py":
(https://github.com/bitcoin/bitcoin/issues/29030#issuecomment-1847572942)
Seems like asserting for the debug log message "Added connection peer=" is insufficient for ensuring that this new connection will show up in a following `getpeerinfo()` call, as the debug message is written in the `CNode` ctor, which means it hasn't been added to `CConnman.m_nodes` at this point yet. If I'm not missing anything, then using the newly introduced `wait_for_new_connection` helper instead of asserting the debug log should fix that.
(https://github.com/bitcoin/bitcoin/issues/29030#issuecomment-1847572942)
Seems like asserting for the debug log message "Added connection peer=" is insufficient for ensuring that this new connection will show up in a following `getpeerinfo()` call, as the debug message is written in the `CNode` ctor, which means it hasn't been added to `CConnman.m_nodes` at this point yet. If I'm not missing anything, then using the newly introduced `wait_for_new_connection` helper instead of asserting the debug log should fix that.
💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1847574299)
Hmm. I have something working, but as usual, MSVC is the odd case. Looking at the current code, it's not clear to me how MSVC ends up doing byteswaps because we don't have any detection for it. But I also doubt that it's finding the functions in `<byteswap.h>`.
So I think it's possible that MSVC is always taking the slow path? If that's the case, fixing this here would give it a big speedup.
@hebasto could you check with MSVC? It should be easy to check with master with something like:
`
...
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1847574299)
Hmm. I have something working, but as usual, MSVC is the odd case. Looking at the current code, it's not clear to me how MSVC ends up doing byteswaps because we don't have any detection for it. But I also doubt that it's finding the functions in `<byteswap.h>`.
So I think it's possible that MSVC is always taking the slow path? If that's the case, fixing this here would give it a big speedup.
@hebasto could you check with MSVC? It should be easy to check with master with something like:
`
...
💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1847634783)
Updated 778c0214d25e5a012c61251d592257ae19805255 -> 8b21f41335dc837b36ea863156605d092f47a0ab ([`pr/rmutil.2`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.2) -> [`pr/rmutil.3`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/rmutil.2..pr/rmutil.3)) to fix clang-tidy error https://cirrus-ci.com/task/4602567305461760. Also switched to `using` declarations more places to avoid creating conflicts and make the PR easier to
...
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-1847634783)
Updated 778c0214d25e5a012c61251d592257ae19805255 -> 8b21f41335dc837b36ea863156605d092f47a0ab ([`pr/rmutil.2`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.2) -> [`pr/rmutil.3`](https://github.com/ryanofsky/bitcoin/commits/pr/rmutil.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/rmutil.2..pr/rmutil.3)) to fix clang-tidy error https://cirrus-ci.com/task/4602567305461760. Also switched to `using` declarations more places to avoid creating conflicts and make the PR easier to
...
💬 hebasto commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1847650626)
@theuni
https://github.com/bitcoin/bitcoin/blob/3e691258d8789a4a89cce42e7e71b130491594d7/build_msvc/bitcoin_config.h.in#L69
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1847650626)
@theuni
https://github.com/bitcoin/bitcoin/blob/3e691258d8789a4a89cce42e7e71b130491594d7/build_msvc/bitcoin_config.h.in#L69
💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1847650629)
Force pushed to remove the fix to `doc/benchmarking.md` as suggested by https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1420176846
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1847650629)
Force pushed to remove the fix to `doc/benchmarking.md` as suggested by https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1420176846
💬 theuni commented on pull request "[POC] C++20 `std::endian`":
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1847657957)
> @theuni
>
> https://github.com/bitcoin/bitcoin/blob/3e691258d8789a4a89cce42e7e71b130491594d7/build_msvc/bitcoin_config.h.in#L69
Thanks! https://github.com/theuni/bitcoin/commit/bbc72033128b6496dd3a92432eadbc4e75aaffe2 ready for PR if it indeed provides a speedup. I don't see how it couldn't :)
(https://github.com/bitcoin/bitcoin/pull/28674#issuecomment-1847657957)
> @theuni
>
> https://github.com/bitcoin/bitcoin/blob/3e691258d8789a4a89cce42e7e71b130491594d7/build_msvc/bitcoin_config.h.in#L69
Thanks! https://github.com/theuni/bitcoin/commit/bbc72033128b6496dd3a92432eadbc4e75aaffe2 ready for PR if it indeed provides a speedup. I don't see how it couldn't :)