💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1961638926)
(I'm fine with the "path\\to\\walletname"-part though).
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1961638926)
(I'm fine with the "path\\to\\walletname"-part though).
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668606129)
Replaced `NONFATAL_UNREACHABLE` with `CHECK_NONFATAL` again by avoiding the intermediate `std::optional`, see https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961627062
Rebased and pushed again, though I haven't figured out the CI issue.
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668606129)
Replaced `NONFATAL_UNREACHABLE` with `CHECK_NONFATAL` again by avoiding the intermediate `std::optional`, see https://github.com/bitcoin/bitcoin/pull/31785#discussion_r1961627062
Rebased and pushed again, though I haven't figured out the CI issue.
💬 vasild commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668649415)
> Remote end closed connection without response
That probably means `bitcoind` crashed.
> node0 stderr ... optional ... Assertion 'this->_M_is_engaged()' failed.
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668649415)
> Remote end closed connection without response
That probably means `bitcoind` crashed.
> node0 stderr ... optional ... Assertion 'this->_M_is_engaged()' failed.
✅ maflcko closed a pull request: "test: Assert unused port to debug intermittent issue 30030"
(https://github.com/bitcoin/bitcoin/pull/31903)
(https://github.com/bitcoin/bitcoin/pull/31903)
💬 maflcko commented on pull request "test: Assert unused port to debug intermittent issue 30030":
(https://github.com/bitcoin/bitcoin/pull/31903#issuecomment-2668650948)
```
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
[08:12:30.628] python3 50589 root 11u IPv4 179599797 0t0 TCP localhost:60000->localhost:19216 (ESTABLISHED)
[08:12:30.628] bitcoind 50594 root 21u IPv4 179704903 0t0 TCP localhost:19216->localhost:60000 (ESTABLISHED)
(https://github.com/bitcoin/bitcoin/pull/31903#issuecomment-2668650948)
```
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
[08:12:30.628] python3 50589 root 11u IPv4 179599797 0t0 TCP localhost:60000->localhost:19216 (ESTABLISHED)
[08:12:30.628] bitcoind 50594 root 21u IPv4 179704903 0t0 TCP localhost:19216->localhost:60000 (ESTABLISHED)
📝 l0rinc opened a pull request: "refactor: modernize outdated trait patterns using helper aliases (C++14)"
(https://github.com/bitcoin/bitcoin/pull/31904)
The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11).
The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly.
I've modified the instances I found in the codebase one-by-o
...
(https://github.com/bitcoin/bitcoin/pull/31904)
The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11).
The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly.
I've modified the instances I found in the codebase one-by-o
...
⚠️ goodthebest opened an issue: "ERROR: AcceptBlockHeader: prev block not found"
(https://github.com/bitcoin/bitcoin/issues/31905)
I noticed in debug.log there are numerous such errors. I'm wondering is it meant to be on local server's blockchain storage or it's kind of network error? Blockchain apparently fully synced yet these errors?
2025-02-16T03:43:19Z ERROR: AcceptBlockHeader: prev block not found
2025-02-16T03:43:27Z ERROR: AcceptBlockHeader: prev block not found
2025-02-16T03:44:01Z ERROR: AcceptBlockHeader: prev block not found
2025-02-16T03:44:37Z ERROR: AcceptBlockHeader: prev block not found
2025-02-16T03:45:2
...
(https://github.com/bitcoin/bitcoin/issues/31905)
I noticed in debug.log there are numerous such errors. I'm wondering is it meant to be on local server's blockchain storage or it's kind of network error? Blockchain apparently fully synced yet these errors?
2025-02-16T03:43:19Z ERROR: AcceptBlockHeader: prev block not found
2025-02-16T03:43:27Z ERROR: AcceptBlockHeader: prev block not found
2025-02-16T03:44:01Z ERROR: AcceptBlockHeader: prev block not found
2025-02-16T03:44:37Z ERROR: AcceptBlockHeader: prev block not found
2025-02-16T03:45:2
...
💬 eval-exec commented on pull request "random: move VerifyRNDRRS above InitHardwareRand":
(https://github.com/bitcoin/bitcoin/pull/31902#issuecomment-2668670182)
Why a `CI failed ` label is added?
(https://github.com/bitcoin/bitcoin/pull/31902#issuecomment-2668670182)
Why a `CI failed ` label is added?
🤔 rkrux reviewed a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2626627692)
I reviewed till df87cf2
I had thought about adding the functional test earlier but had ruled it out because I thought technically it's a bug fix and not a functionality per se.
But after reading this test, I'm in favour of this test, makes me think from the POV of what's happening to the node overall.
(https://github.com/bitcoin/bitcoin/pull/31757#pullrequestreview-2626627692)
I reviewed till df87cf2
I had thought about adding the functional test earlier but had ruled it out because I thought technically it's a bug fix and not a functionality per se.
But after reading this test, I'm in favour of this test, makes me think from the POV of what's happening to the node overall.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961582654)
Can add a comment here highlighting that this RPC call is added to ensure the block disconnected notification is handled by the wallet.
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961582654)
Can add a comment here highlighting that this RPC call is added to ensure the block disconnected notification is handled by the wallet.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961643854)
Can add a comment here: `Disconnect tip again`
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961643854)
Can add a comment here: `Disconnect tip again`
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961641424)
```diff
+ # Abort process abruptly to force an unclean shutdown (no flush) so that the best block locator changes are not persisted
```
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961641424)
```diff
+ # Abort process abruptly to force an unclean shutdown (no flush) so that the best block locator changes are not persisted
```
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961637704)
This test passes in my system but one thing that's on my mind is that can this test ever become flaky? Can there be a scenario that the best block locator changes are written to the disk before the node is killed here? Do you think a time delay should be added before killing the node here?
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961637704)
This test passes in my system but one thing that's on my mind is that can this test ever become flaky? Can there be a scenario that the best block locator changes are written to the disk before the node is killed here? Do you think a time delay should be added before killing the node here?
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961669843)
Might not hurt to check for wallet balance again. But more importantly it would be nice to check that the best block hash is not `tip` now like below. Should we stop the node again (cleanly this time) and check this on start?
```diff
+ assert_equal(node.getbestblockhash() == tip, False)
```
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961669843)
Might not hurt to check for wallet balance again. But more importantly it would be nice to check that the best block hash is not `tip` now like below. Should we stop the node again (cleanly this time) and check this on start?
```diff
+ assert_equal(node.getbestblockhash() == tip, False)
```
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961680280)
Interesting that the best block hash at the stage is not the `tip` because of invalidation but not after unclean-stop-start.
```
assert_equal(node.getbestblockhash() == tip, False)
```
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961680280)
Interesting that the best block hash at the stage is not the `tip` because of invalidation but not after unclean-stop-start.
```
assert_equal(node.getbestblockhash() == tip, False)
```
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961623606)
Might be in the nit-land but I find reading `25` here distracting. I had to figure out why it's 25 that is because in regtest the subsidy halving interval is 150 and initially a 199 blocks long chain is created in the regtest. The block count at this stage is 215 which is because few blocks were created in the preceding test in this file, thereby making this test dependent on the changes of that test as well.
IMO, a simple balance greater than 0 check is sufficient.
Also, I am inclined to cr
...
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961623606)
Might be in the nit-land but I find reading `25` here distracting. I had to figure out why it's 25 that is because in regtest the subsidy halving interval is 150 and initially a 199 blocks long chain is created in the regtest. The block count at this stage is 215 which is because few blocks were created in the preceding test in this file, thereby making this test dependent on the changes of that test as well.
IMO, a simple balance greater than 0 check is sufficient.
Also, I am inclined to cr
...
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961647929)
Re: https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1956236842
Bringing it back again because I feel besides the robustness `IsCoinBase()` brings in here, we can also get rid of introducing `index` here that is not used elsewhere.
LMK your thoughts.
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961647929)
Re: https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1956236842
Bringing it back again because I feel besides the robustness `IsCoinBase()` brings in here, we can also get rid of introducing `index` here that is not used elsewhere.
LMK your thoughts.
💬 rkrux commented on pull request "wallet: fix crash on double block disconnection":
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961687763)
`Test that wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown`
(https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1961687763)
`Test that wallet doesn't crash due to a duplicate block disconnection event after an unclean shutdown`
💬 tnull commented on pull request "Fix field name styling in `submitpackage` RPC results":
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2668679262)
Alright, I now updated this PR to adjust all `submitpackage` result field names that previously used dash-case. As it seemed a bit odd to have the same field with different styling as part of `testmempoolaccept`, I now also added a commit aligning the fields there, too, but please let me know if I should simply drop that commit if we wouldn't want to change that RPC API as well.
> fwiw, if we think of pre-29 as the last chance to be liberal with changing API, I do think unifying it within `su
...
(https://github.com/bitcoin/bitcoin/pull/31900#issuecomment-2668679262)
Alright, I now updated this PR to adjust all `submitpackage` result field names that previously used dash-case. As it seemed a bit odd to have the same field with different styling as part of `testmempoolaccept`, I now also added a commit aligning the fields there, too, but please let me know if I should simply drop that commit if we wouldn't want to change that RPC API as well.
> fwiw, if we think of pre-29 as the last chance to be liberal with changing API, I do think unifying it within `su
...
💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668707017)
M_engaged seems to be gcc specific for `std::optional`: https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/optional#L291
It presumably happens when you dereference it in a bad way, but then why doesn't it crash when compiled with clang (as on my macOS machine)?
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668707017)
M_engaged seems to be gcc specific for `std::optional`: https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/optional#L291
It presumably happens when you dereference it in a bad way, but then why doesn't it crash when compiled with clang (as on my macOS machine)?