⚠️ gianlucamazza opened an issue: "bitcoin core crashes and restarts syncing from beginning "
(https://github.com/bitcoin/bitcoin/issues/28795)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I am consistently encountering a `LevelDB read failure: Corruption: block checksum mismatch` error when attempting to synchronize the Bitcoin blockchain on multiple Raspberry Pi devices.
The corruption error happens always around 2016-01-28T16:10:24Z as far I remember. Any idea why this happens?
### Expected behaviour
the full node should fully syncronize
### Steps to reproduce
bitcoi
...
(https://github.com/bitcoin/bitcoin/issues/28795)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I am consistently encountering a `LevelDB read failure: Corruption: block checksum mismatch` error when attempting to synchronize the Bitcoin blockchain on multiple Raspberry Pi devices.
The corruption error happens always around 2016-01-28T16:10:24Z as far I remember. Any idea why this happens?
### Expected behaviour
the full node should fully syncronize
### Steps to reproduce
bitcoi
...
🤔 theStack reviewed a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-1713893741)
Concept ACK
As a follow-up, it might be nice if we could somehow add coverage for the "other endianness" code path in the tests. Did that manually with the following patch (creates the BDB database in big-endian format, while I'm running a little-endian system):
```diff
diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
index 9ea43ca67c..945e12ebdb 100644
--- a/src/wallet/bdb.cpp
+++ b/src/wallet/bdb.cpp
@@ -389,6 +389,7 @@ void BerkeleyDatabase::Open()
}
...
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-1713893741)
Concept ACK
As a follow-up, it might be nice if we could somehow add coverage for the "other endianness" code path in the tests. Did that manually with the following patch (creates the BDB database in big-endian format, while I'm running a little-endian system):
```diff
diff --git a/src/wallet/bdb.cpp b/src/wallet/bdb.cpp
index 9ea43ca67c..945e12ebdb 100644
--- a/src/wallet/bdb.cpp
+++ b/src/wallet/bdb.cpp
@@ -389,6 +389,7 @@ void BerkeleyDatabase::Open()
}
...
💬 theStack commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1382488042)
(in commit a02db44419148984d726f95fd66030b9a40fc0bb) readability nit: could also use `std::get` here, as the RHS of the OR conditoin is only executed if the variant holds the right data type:
```suggestion
if (!std::holds_alternative<DataRecord>(page.records.at(0)) || std::get<DataRecord>(page.records.at(0)).data != SUBDATABASE_NAME) {
```
(same for the two `page.records.at(1)` instances a few lines below)
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1382488042)
(in commit a02db44419148984d726f95fd66030b9a40fc0bb) readability nit: could also use `std::get` here, as the RHS of the OR conditoin is only executed if the variant holds the right data type:
```suggestion
if (!std::holds_alternative<DataRecord>(page.records.at(0)) || std::get<DataRecord>(page.records.at(0)).data != SUBDATABASE_NAME) {
```
(same for the two `page.records.at(1)` instances a few lines below)
💬 theStack commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1382487576)
(in commit b7b90daf46ae840d9ab089ac438f71f7b79dbf5a) could avoid duplicate std::map lookup by using `.find` instead:
```suggestion
const auto it{m_database.m_records.find(key_data)};
if (it == m_database.m_records.end()) {
return false;
}
auto val = it->second;
```
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1382487576)
(in commit b7b90daf46ae840d9ab089ac438f71f7b79dbf5a) could avoid duplicate std::map lookup by using `.find` instead:
```suggestion
const auto it{m_database.m_records.find(key_data)};
if (it == m_database.m_records.end()) {
return false;
}
auto val = it->second;
```
📝 hebasto opened a pull request: "ci: Drop no longer needed "Fix Visual Studio installation" step"
(https://github.com/bitcoin/bitcoin/pull/28796)
The underlying issue has been [fixed](https://github.com/actions/runner-images/pull/8686) in the image version 20231029.
(https://github.com/bitcoin/bitcoin/pull/28796)
The underlying issue has been [fixed](https://github.com/actions/runner-images/pull/8686) in the image version 20231029.
💬 pinheadmz commented on issue "bitcoin core crashes and restarts syncing from beginning ":
(https://github.com/bitcoin/bitcoin/issues/28795#issuecomment-1793707433)
You're running Bitcoin Core on a Raspberry Pi which can work with the right setup. Looks like you are storing blocks on an external drive of some kind, but the chainstate database might still be stored on the MicroSD card (?) and got corrupted.
It's a little hard to tell from your output what kind of external drive you have but assuming its a good quality SSD connected by USB3 to the Pi, you might consider mounting it directly to `/home/pi/.bitcoin` (this is what I do). Then all block, index,
...
(https://github.com/bitcoin/bitcoin/issues/28795#issuecomment-1793707433)
You're running Bitcoin Core on a Raspberry Pi which can work with the right setup. Looks like you are storing blocks on an external drive of some kind, but the chainstate database might still be stored on the MicroSD card (?) and got corrupted.
It's a little hard to tell from your output what kind of external drive you have but assuming its a good quality SSD connected by USB3 to the Pi, you might consider mounting it directly to `/home/pi/.bitcoin` (this is what I do). Then all block, index,
...
💬 Retropex commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1793719771)
> Node runners need a builtin option to ignore all modern forms of datacarrying so they don't have to resort to manually patching their nodes.
In addition, it is important to note that if developers do not help node runners defend themselves against this attack, they may have to resort to misappropriate means to strengthen their defense. Although the use of a pre-SegWit node is a defense option, it can have harmful consequences for the entire network.
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1793719771)
> Node runners need a builtin option to ignore all modern forms of datacarrying so they don't have to resort to manually patching their nodes.
In addition, it is important to note that if developers do not help node runners defend themselves against this attack, they may have to resort to misappropriate means to strengthen their defense. Although the use of a pre-SegWit node is a defense option, it can have harmful consequences for the entire network.
💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1793751225)
Nice, I'm going to re-review this once I'm done with 25273
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1793751225)
Nice, I'm going to re-review this once I'm done with 25273
💬 andrewtoth commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1793773276)
ACK 5628ac55be42c5450c6817ba8dcfe463ceda32a9
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1793773276)
ACK 5628ac55be42c5450c6817ba8dcfe463ceda32a9
💬 dergoegge commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1382611940)
Isn't the tunnel already created at this point? Afaik the tunnel creation is handled by the i2p gateway (e.g. i2pd) not on our side.
I'm also not sure what we are protecting against here. Assuming tunnel creation is sufficiently DoSy (which it shouldn't be), this isn't enough to prevent attacks as an attacker could just not connect from the same address.
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1382611940)
Isn't the tunnel already created at this point? Afaik the tunnel creation is handled by the i2p gateway (e.g. i2pd) not on our side.
I'm also not sure what we are protecting against here. Assuming tunnel creation is sufficiently DoSy (which it shouldn't be), this isn't enough to prevent attacks as an attacker could just not connect from the same address.
🤔 S3RK reviewed a pull request: "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction"
(https://github.com/bitcoin/bitcoin/pull/25273#pullrequestreview-1705662758)
Approach ACK.
Left inline comments with more details.
It also would be nice to update PR description to explain what defects it fixes, i.e. expand "This can have an effect on whether and how anti-fee-sniping works." by describing potential bugs or negative effects.
(https://github.com/bitcoin/bitcoin/pull/25273#pullrequestreview-1705662758)
Approach ACK.
Left inline comments with more details.
It also would be nice to update PR description to explain what defects it fixes, i.e. expand "This can have an effect on whether and how anti-fee-sniping works." by describing potential bugs or negative effects.
💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1378511724)
Can figure out how is it set currently?
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1378511724)
Can figure out how is it set currently?
💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382610792)
in 03420020b6b4958ee6765be718724b89cd174d5c
`locktime` doesn't belong to `CoinControl` because it has nothing to do with coins being spent. I'd prefer if we keep `CoinControl` scoped down to only containing data necessary to select and spend coins.
Same is true for some other fields in `CoinControl`. Maybe we should extract parameters not related to coins into a separate struct?
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382610792)
in 03420020b6b4958ee6765be718724b89cd174d5c
`locktime` doesn't belong to `CoinControl` because it has nothing to do with coins being spent. I'd prefer if we keep `CoinControl` scoped down to only containing data necessary to select and spend coins.
Same is true for some other fields in `CoinControl`. Maybe we should extract parameters not related to coins into a separate struct?
💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382610856)
in c8544398fb945b2402274981b22c70fdd8a67b9e
Same comment as for `locktime`
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382610856)
in c8544398fb945b2402274981b22c70fdd8a67b9e
Same comment as for `locktime`
💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382612557)
in 53cb734591cabb37fc6e3d147a4715203cf15ddb
There is redundancy between `m_script_sig ` + `m_script_witness ` and `m_weight`. One can set all of them and reach and inconsistent state, which will lead to error.
Can we make the interface better?
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382612557)
in 53cb734591cabb37fc6e3d147a4715203cf15ddb
There is redundancy between `m_script_sig ` + `m_script_witness ` and `m_weight`. One can set all of them and reach and inconsistent state, which will lead to error.
Can we make the interface better?
💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382611119)
I'm not sure about 49ce0fe1e2ec87508aea537d17e6bb13791be1aa and probably would prefer to drop it.
It makes interface less clear. It's not obvious that I need to call `SetTxOut` to make input as external.
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382611119)
I'm not sure about 49ce0fe1e2ec87508aea537d17e6bb13791be1aa and probably would prefer to drop it.
It makes interface less clear. It's not obvious that I need to call `SetTxOut` to make input as external.
💬 S3RK commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382612097)
in 59a7c7416313693a3c8219363d9ffee1e2b86d4d
Naming is hard. We already have struct `PreSelectedInputs` which is quite confusing as it refers to different data.
`PreSelectedInputs` is basically `set<COutput>`. It seems we should find a better name for `PreSelectedInputs`. Maybe rename it to `PreSelectedCoins`?
It'd be nice to have consistent terms to refer to different data.
How about we use `input` when we talk about tx.in and related info.
And we use `coin` when its enriched data in
...
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1382612097)
in 59a7c7416313693a3c8219363d9ffee1e2b86d4d
Naming is hard. We already have struct `PreSelectedInputs` which is quite confusing as it refers to different data.
`PreSelectedInputs` is basically `set<COutput>`. It seems we should find a better name for `PreSelectedInputs`. Maybe rename it to `PreSelectedCoins`?
It'd be nice to have consistent terms to refer to different data.
How about we use `input` when we talk about tx.in and related info.
And we use `coin` when its enriched data in
...
💬 maflcko commented on pull request "ci: Drop no longer needed "Fix Visual Studio installation" step":
(https://github.com/bitcoin/bitcoin/pull/28796#issuecomment-1793797549)
lgtm ACK 5bd1b8d4f1ab4dd947c5e93712ba47e14a0fe2da
(https://github.com/bitcoin/bitcoin/pull/28796#issuecomment-1793797549)
lgtm ACK 5bd1b8d4f1ab4dd947c5e93712ba47e14a0fe2da
💬 maflcko commented on pull request "Embedding ASMap files as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1793798658)
How much data would this add per year to the repo, assuming updates happen?
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1793798658)
How much data would this add per year to the repo, assuming updates happen?
💬 maflcko commented on pull request "depends: Bump to capnproto-c++-1.0.1":
(https://github.com/bitcoin/bitcoin/pull/28735#issuecomment-1793799659)
I don't think the CI win64-cross build has multiprocess enabled. There is only one non-windows one (multiprocess, i686, DEBUG). Also, I don't think guix has multiprocess enabled, and probably won't have it enabled until at least mid-next year. Anyone is welcome to fix the win64-cross multiprocess build, and even enable it in CI or guix, if they want to.
(https://github.com/bitcoin/bitcoin/pull/28735#issuecomment-1793799659)
I don't think the CI win64-cross build has multiprocess enabled. There is only one non-windows one (multiprocess, i686, DEBUG). Also, I don't think guix has multiprocess enabled, and probably won't have it enabled until at least mid-next year. Anyone is welcome to fix the win64-cross multiprocess build, and even enable it in CI or guix, if they want to.