📝 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.
⚠️ TheBlueMatt opened an issue: "Wallet Missing Balances/Unspent"
(https://github.com/bitcoin/bitcoin/issues/28797)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Wallet is missing the balance for txid d49589ccd8488c56fbf12aee2652f760e1e84e6da1edd3dcf98bf6d556151396. The output address (and input address) are mine:
```
getaddressinfo bc1qag6gtevhvuhafph4d73ajlj5vyhh7k4xff0p4f
{
"address": "bc1qag6gtevhvuhafph4d73ajlj5vyhh7k4xff0p4f",
"scriptPubKey": "0014ea3485e597672fd486f56fa3d97e54612f7f5aa6",
"ismine": true,
"solvable": true,
...
(https://github.com/bitcoin/bitcoin/issues/28797)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Wallet is missing the balance for txid d49589ccd8488c56fbf12aee2652f760e1e84e6da1edd3dcf98bf6d556151396. The output address (and input address) are mine:
```
getaddressinfo bc1qag6gtevhvuhafph4d73ajlj5vyhh7k4xff0p4f
{
"address": "bc1qag6gtevhvuhafph4d73ajlj5vyhh7k4xff0p4f",
"scriptPubKey": "0014ea3485e597672fd486f56fa3d97e54612f7f5aa6",
"ismine": true,
"solvable": true,
...
📝 hebasto opened a pull request: "build: Drop no longer needed MSVC warning suppressions"
(https://github.com/bitcoin/bitcoin/pull/28798)
(https://github.com/bitcoin/bitcoin/pull/28798)
💬 sipa commented on pull request "Embedding ASMap files as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1793804415)
@maflcko The asmap file added here is around 1.6 MiB, so assuming we update for every 6 months, probably a bit over 3 MiB per year?
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-1793804415)
@maflcko The asmap file added here is around 1.6 MiB, so assuming we update for every 6 months, probably a bit over 3 MiB per year?
🚀 fanquake merged a pull request: "ci: Drop no longer needed "Fix Visual Studio installation" step"
(https://github.com/bitcoin/bitcoin/pull/28796)
(https://github.com/bitcoin/bitcoin/pull/28796)