💬 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)?
💬 l0rinc commented on issue "CI: Failed pulls from docker.io causing jobs to fail":
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2668710356)
Happened again in https://github.com/bitcoin/bitcoin/pull/31904/checks?check_run_id=37470244364
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2668710356)
Happened again in https://github.com/bitcoin/bitcoin/pull/31904/checks?check_run_id=37470244364
💬 rkrux commented on issue "wallet: wrong balance and crash after reorg and unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2668723047)
> I think the root of the problems is that changes to the wallet db are synced immediately, but the best block locator isn't.
Is this not where the chainstate related changes are flushed to disk? https://github.com/bitcoin/bitcoin/blob/28.x/src/validation.cpp#L3069
And here block disconnected notification fired that is listened to by the wallet: https://github.com/bitcoin/bitcoin/blob/28.x/src/validation.cpp#L3087
(https://github.com/bitcoin/bitcoin/issues/31824#issuecomment-2668723047)
> I think the root of the problems is that changes to the wallet db are synced immediately, but the best block locator isn't.
Is this not where the chainstate related changes are flushed to disk? https://github.com/bitcoin/bitcoin/blob/28.x/src/validation.cpp#L3069
And here block disconnected notification fired that is listened to by the wallet: https://github.com/bitcoin/bitcoin/blob/28.x/src/validation.cpp#L3087
💬 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-2668744831)
I would guess the optional is dereferenced when it is nullopt and this is not compiler related. Maybe it only happens on CI due to some timing specifics? CI failures that don't repro locally are the most tedious to nail down. Try adding a lot of `LogError("reached line 123, relevant values %d %s %whatever", ...)` and then inspect the CI log for those.
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668744831)
I would guess the optional is dereferenced when it is nullopt and this is not compiler related. Maybe it only happens on CI due to some timing specifics? CI failures that don't repro locally are the most tedious to nail down. Try adding a lot of `LogError("reached line 123, relevant values %d %s %whatever", ...)` and then inspect the CI log for those.
💬 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-2668747128)
Trying `.value()` instead of `*`...
https://github.com/bitcoin/bitcoin/compare/51aa29ede7bd9a1897f3cf240e70ef180849591d..3b877e6718a916402adf8dcb141da01064289a6b
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668747128)
Trying `.value()` instead of `*`...
https://github.com/bitcoin/bitcoin/compare/51aa29ede7bd9a1897f3cf240e70ef180849591d..3b877e6718a916402adf8dcb141da01064289a6b
💬 maflcko commented on issue "CI: Failed pulls from docker.io causing jobs to fail":
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2668753666)
`24.04` is already cached, so it is unclear why `noble` gets rate-limited. I guess that'd be a reason to use the same name consistently.
Absent that, a real fix would still be good.
(https://github.com/bitcoin/bitcoin/issues/31797#issuecomment-2668753666)
`24.04` is already cached, so it is unclear why `noble` gets rate-limited. I guess that'd be a reason to use the same name consistently.
Absent that, a real fix would still be good.
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1961743938)
fwiw, here's what I landed on for `test_orphanage_dos_many`:
(1) send 51 of the same orphan from 60 peers (51 orphans, 3060 announcements). `sync_with_ping` and wait until at least one of the orphans is in orphanage
(2) send the 1p1c, which is a small orphan (not 100kvb because prior to increasing `-maxorphantxs`, this peer's memory DoS score could be higher than the other peers' CPU DoS scores and get the tx evicted. Better to compare apples to apples).
(3) send 51 unique orphans from 40 pee
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1961743938)
fwiw, here's what I landed on for `test_orphanage_dos_many`:
(1) send 51 of the same orphan from 60 peers (51 orphans, 3060 announcements). `sync_with_ping` and wait until at least one of the orphans is in orphanage
(2) send the 1p1c, which is a small orphan (not 100kvb because prior to increasing `-maxorphantxs`, this peer's memory DoS score could be higher than the other peers' CPU DoS scores and get the tx evicted. Better to compare apples to apples).
(3) send 51 unique orphans from 40 pee
...
💬 ryanofsky 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-2668757560)
This failure is strange. I tried to reproduce the failure from https://cirrus-ci.com/task/6207546920271872?logs=ci#L2923:
```c++
node0 stderr /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/optional:479: _Tp &std::_Optional_base_impl<interfaces::BlockRef, std::_Optional_base<interfaces::BlockRef>>::_M_get() [_Tp = interfaces::BlockRef, _Dp = std::_Optional_base<interfaces::BlockRef>]: Assertion 'this->_M_is_engaged()' failed.
```
using gcc 13.3.0, checking out commit
...
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2668757560)
This failure is strange. I tried to reproduce the failure from https://cirrus-ci.com/task/6207546920271872?logs=ci#L2923:
```c++
node0 stderr /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/optional:479: _Tp &std::_Optional_base_impl<interfaces::BlockRef, std::_Optional_base<interfaces::BlockRef>>::_M_get() [_Tp = interfaces::BlockRef, _Dp = std::_Optional_base<interfaces::BlockRef>]: Assertion 'this->_M_is_engaged()' failed.
```
using gcc 13.3.0, checking out commit
...
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2668766678)
The changes here have surprisingly little effect on the included benchmarks:
* [bench] TxOrphanage::LimitOrphans
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 68,742,718.06 | 14.55 | 0.2% | 10.59 | `OrphanageEraseForBlockSinglePeer`
| 8,857.40 | 112,899.93 | 0.3% | 10.86 | `OrphanageEvictionManyPeers`
| 1,544,919.6
...
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2668766678)
The changes here have surprisingly little effect on the included benchmarks:
* [bench] TxOrphanage::LimitOrphans
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 68,742,718.06 | 14.55 | 0.2% | 10.59 | `OrphanageEraseForBlockSinglePeer`
| 8,857.40 | 112,899.93 | 0.3% | 10.86 | `OrphanageEvictionManyPeers`
| 1,544,919.6
...
💬 purpleKarrot commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961755502)
You can use `std::is_same_v` here!
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961755502)
You can use `std::is_same_v` here!
💬 maflcko commented on pull request "ci: optionally use local docker build cache":
(https://github.com/bitcoin/bitcoin/pull/31545#discussion_r1961758582)
I wonder if any of this could fail? While you claimed that docker can handle an parallel `rm -rf "${DOCKER_BUILD_CACHE_OLD_DIR}"`, I wonder if the `mv` and `rm` could fail when done concurrently. I guess `rm -rf` never fails, and `mv` is atomic. However, running two `mv` at the same time seems like one should fail. And running `mv` and `rm` at the same time, seem like it could corrupt the cache.
(https://github.com/bitcoin/bitcoin/pull/31545#discussion_r1961758582)
I wonder if any of this could fail? While you claimed that docker can handle an parallel `rm -rf "${DOCKER_BUILD_CACHE_OLD_DIR}"`, I wonder if the `mv` and `rm` could fail when done concurrently. I guess `rm -rf` never fails, and `mv` is atomic. However, running two `mv` at the same time seems like one should fail. And running `mv` and `rm` at the same time, seem like it could corrupt the cache.
💬 l0rinc commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14)":
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961764149)
I am, just doing it commit-by-commit: https://github.com/bitcoin/bitcoin/pull/31904/files#diff-4b1ae2ce314ef1f2d4cb4f2dbf7a87a0c299c942782fa734e9e80fed8324faa6R76
(https://github.com/bitcoin/bitcoin/pull/31904#discussion_r1961764149)
I am, just doing it commit-by-commit: https://github.com/bitcoin/bitcoin/pull/31904/files#diff-4b1ae2ce314ef1f2d4cb4f2dbf7a87a0c299c942782fa734e9e80fed8324faa6R76
👍 vasild approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2626937531)
ACK 2d65225c4696dcc8312d546eb67f4eae11b9b8fe modulo the comment below
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2626937531)
ACK 2d65225c4696dcc8312d546eb67f4eae11b9b8fe modulo the comment below
💬 vasild commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1961755597)
This was `return check_tip_changed() || chainman().m_interrupt;` when the lambda `check_tip_changed()` was used. Now that the lambda is expanded here, the `chainman().m_interrupt` condition was lost. I think it should be:
```suggestion
return tip_changed || chainman().m_interrupt;
```
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1961755597)
This was `return check_tip_changed() || chainman().m_interrupt;` when the lambda `check_tip_changed()` was used. Now that the lambda is expanded here, the `chainman().m_interrupt` condition was lost. I think it should be:
```suggestion
return tip_changed || chainman().m_interrupt;
```
💬 purpleKarrot commented on pull request "refactor: modernize outdated trait patterns using helper aliases (C++14)":
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2668789181)
This change could be automated with clang-tidy's [`modernize-type-traits`](https://clang.llvm.org/extra/clang-tidy/checks/modernize/type-traits.html) check. There are more places that can be modernized with this.
(https://github.com/bitcoin/bitcoin/pull/31904#issuecomment-2668789181)
This change could be automated with clang-tidy's [`modernize-type-traits`](https://clang.llvm.org/extra/clang-tidy/checks/modernize/type-traits.html) check. There are more places that can be modernized with this.