💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1612073178)
"single transaction subpackage"?
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1612073178)
"single transaction subpackage"?
💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1612076306)
That indeed looks better.
The *ptr variables are no longer actually pointers, so I suggest renaming them:
msgptr -> msg_pos
within entry - within message
saptr -> sa_pos
next_msgptr -> next_msg_pos
Let's set and check next_msgptr _before_ saptr and make it const.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1612076306)
That indeed looks better.
The *ptr variables are no longer actually pointers, so I suggest renaming them:
msgptr -> msg_pos
within entry - within message
saptr -> sa_pos
next_msgptr -> next_msg_pos
Let's set and check next_msgptr _before_ saptr and make it const.
💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1612076482)
> If you are sure that if detail_fread(dst) == dst.size() then ferror(m_file) will always be false or feel strongly against this then I could drop the commit https://github.com/bitcoin/bitcoin/commit/de23848eed5437f5e4dc6ccdc38b40cc58738ead util: check for error after reading from a file.
I don't feel strongly in one way or another. I am just saying that with the currently available information, it is not possible to confidently review this and know it is a good change. At a minimum, it shoul
...
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1612076482)
> If you are sure that if detail_fread(dst) == dst.size() then ferror(m_file) will always be false or feel strongly against this then I could drop the commit https://github.com/bitcoin/bitcoin/commit/de23848eed5437f5e4dc6ccdc38b40cc58738ead util: check for error after reading from a file.
I don't feel strongly in one way or another. I am just saying that with the currently available information, it is not possible to confidently review this and know it is a good change. At a minimum, it shoul
...
💬 Sjors commented on pull request "build: LLD based macOS toolchain":
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2127723578)
Tested e8c25e8a35e333e90514945c592557615641553f: the guix build, a local depends build and a local normal build on Intel macOS 14.5. Tested a normal build on macOS 13.6.7.
Guix hashes (Ubuntu, AMD), matches what @TheCharlatan and @fanquake found above.
```
00becde2dd12878e3b9f50f27899a6a8b752343dade7c71781632715c3001473 guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/SHA256SUMS.part
a685b9cee54014e74639be1e8db2d55b7c008fdb3b31c1c708c364a49b56759a guix-build-e8c25e8a35e3/output/aarch64
...
(https://github.com/bitcoin/bitcoin/pull/21778#issuecomment-2127723578)
Tested e8c25e8a35e333e90514945c592557615641553f: the guix build, a local depends build and a local normal build on Intel macOS 14.5. Tested a normal build on macOS 13.6.7.
Guix hashes (Ubuntu, AMD), matches what @TheCharlatan and @fanquake found above.
```
00becde2dd12878e3b9f50f27899a6a8b752343dade7c71781632715c3001473 guix-build-e8c25e8a35e3/output/aarch64-linux-gnu/SHA256SUMS.part
a685b9cee54014e74639be1e8db2d55b7c008fdb3b31c1c708c364a49b56759a guix-build-e8c25e8a35e3/output/aarch64
...
💬 apulsifer commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2127734105)
xfs is used on the root drive of every Amazon EC2 instance running Amazon Linux. If xfs on Amazon EC2 were the problem, a lot of of critical infrastructure would be failing right now. And as I mentioned, these machines are also running bitcoin cash in an almost an identical configuration (data file path and ports changed) and it has had zero problems. Since the data corruption only happens about once per week when running on mainnet, I think figuring this problem out will probably take a customi
...
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2127734105)
xfs is used on the root drive of every Amazon EC2 instance running Amazon Linux. If xfs on Amazon EC2 were the problem, a lot of of critical infrastructure would be failing right now. And as I mentioned, these machines are also running bitcoin cash in an almost an identical configuration (data file path and ports changed) and it has had zero problems. Since the data corruption only happens about once per week when running on mainnet, I think figuring this problem out will probably take a customi
...
💬 mzumsande commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1612109695)
Currently we abort with an assert in the next line after the log, so the expectation in master is that this shouldn't happen outside of db corruption. I couldn't find any reports on the internet of users that have run into this error / assert, so it probably doesn't happen very often in practice.
But theoretically speaking, if the coins db gets corrupted, I guess anything can happen depending on the specifics of the corruption?
While working on this PR, I initially went with the different ap
...
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1612109695)
Currently we abort with an assert in the next line after the log, so the expectation in master is that this shouldn't happen outside of db corruption. I couldn't find any reports on the internet of users that have run into this error / assert, so it probably doesn't happen very often in practice.
But theoretically speaking, if the coins db gets corrupted, I guess anything can happen depending on the specifics of the corruption?
While working on this PR, I initially went with the different ap
...
📝 theStack opened a pull request: "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)"
(https://github.com/bitcoin/bitcoin/pull/30162)
MiniWallet allows to create padded transactions that are equal or slightly above a certain `target_weight` (first introduced in PR #25379, commit 63cfa4ce88cac26abcacf3b243916e722ab17d75), which can be useful especially for mempool-related tests, e.g. for policy limit checks or scenarios to trigger mempool eviction. Currently the `target_weight` parameter doesn't play together with `fee_rate` though, as the fee calculation is incorrectly based on the tx vsize before the padding output is added,
...
(https://github.com/bitcoin/bitcoin/pull/30162)
MiniWallet allows to create padded transactions that are equal or slightly above a certain `target_weight` (first introduced in PR #25379, commit 63cfa4ce88cac26abcacf3b243916e722ab17d75), which can be useful especially for mempool-related tests, e.g. for policy limit checks or scenarios to trigger mempool eviction. Currently the `target_weight` parameter doesn't play together with `fee_rate` though, as the fee calculation is incorrectly based on the tx vsize before the padding output is added,
...
💬 luke-jr commented on pull request "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection":
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2127782169)
This one-line change works without anything else:
https://github.com/bitcoin-core/gui/pull/815#pullrequestreview-2072577992
(https://github.com/bitcoin-core/gui/pull/815#issuecomment-2127782169)
This one-line change works without anything else:
https://github.com/bitcoin-core/gui/pull/815#pullrequestreview-2072577992
💬 edilmedeiros commented on pull request "contrib/signet/miner updates":
(https://github.com/bitcoin/bitcoin/pull/28417#discussion_r1612137178)
I have an open PR (#30130) with an associated issue (#30102) that touches this code. I'm still reviewing this PR, but in the meanwhile do you think it would be a good idea to rebase mine on top of this? As it is, I believe it can't be cherry-picked.
In summary, `finish_block` will call `bitcoin-util` to grind PoW, which can fail if it exhausts the nonce search space. This will be (extremely) rarely seem in signets that keep difficulty low, but it will be quite common if someone wants to keep
...
(https://github.com/bitcoin/bitcoin/pull/28417#discussion_r1612137178)
I have an open PR (#30130) with an associated issue (#30102) that touches this code. I'm still reviewing this PR, but in the meanwhile do you think it would be a good idea to rebase mine on top of this? As it is, I believe it can't be cherry-picked.
In summary, `finish_block` will call `bitcoin-util` to grind PoW, which can fail if it exhausts the nonce search space. This will be (extremely) rarely seem in signets that keep difficulty low, but it will be quite common if someone wants to keep
...
⚠️ zefir-k opened an issue: "prune shall not delete blocks it did not download"
(https://github.com/bitcoin/bitcoin/issues/30163)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I operate a full node on an embedded system with datadir being on a local SSD but the blocks are stored on an external USB-HDD, which is often referred to be a well balanced compromise of speedy chainstate tracking and heavy blockchain data.
When setting up new nodes I usually build bitcoind from source and for a quick upstart I use to attach the USB-HDD containing the blocks to it, whi
...
(https://github.com/bitcoin/bitcoin/issues/30163)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
I operate a full node on an embedded system with datadir being on a local SSD but the blocks are stored on an external USB-HDD, which is often referred to be a well balanced compromise of speedy chainstate tracking and heavy blockchain data.
When setting up new nodes I usually build bitcoind from source and for a quick upstart I use to attach the USB-HDD containing the blocks to it, whi
...
💬 maflcko commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2127790284)
> Since the data corruption only happens about once per week
Once per week is a lot and if this was a broader problem, I'd assume that more people were complaining. Given that you can consistently reproduce on different machines, this seems like a real bug is somewhere. However, Bitcoin Core is running fine in a lot of other places, so there has to be some hardware or configuration setting (or combination thereof) that triggers this bug on your side. It would be good to know which one it is.
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2127790284)
> Since the data corruption only happens about once per week
Once per week is a lot and if this was a broader problem, I'd assume that more people were complaining. Given that you can consistently reproduce on different machines, this seems like a real bug is somewhere. However, Bitcoin Core is running fine in a lot of other places, so there has to be some hardware or configuration setting (or combination thereof) that triggers this bug on your side. It would be good to know which one it is.
💬 sipa commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2127791221)
There are several BIPs that contain specifications relating to testnet, so perhaps a BIP is the right place to define testnet4? The BIP process predates testnet3, but only by a few months, so I don't think we should see the lack of a testnet3 BIP as an argument against this.
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2127791221)
There are several BIPs that contain specifications relating to testnet, so perhaps a BIP is the right place to define testnet4? The BIP process predates testnet3, but only by a few months, so I don't think we should see the lack of a testnet3 BIP as an argument against this.
💬 mzumsande commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2127797705)
> There is a lot of memory pressure and paging when a block comes in
Could you explain the paging in a bit more detail? Is the node constantly being bombarded with lots of simultaneous RPC calls (which ones?) or is some other interface used?
Also, from your log it appears that this happened while the node was catching up with the tip (almost but not completely synced yet), receiving blocks quickly. Is that typical, or does it usually happen when the node is synced and receives blocks as th
...
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2127797705)
> There is a lot of memory pressure and paging when a block comes in
Could you explain the paging in a bit more detail? Is the node constantly being bombarded with lots of simultaneous RPC calls (which ones?) or is some other interface used?
Also, from your log it appears that this happened while the node was catching up with the tip (almost but not completely synced yet), receiving blocks quickly. Is that typical, or does it usually happen when the node is synced and receives blocks as th
...
🤔 sr-gi reviewed a pull request: "refactor prep for package rbf"
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2061682943)
utACK [2fd34ba](https://github.com/bitcoin/bitcoin/pull/30072/commits/2fd34ba504957331f5a08614b6e1f8317260f04d)
Sorry it took me a while to get familiar with the validation logic. I left some non-blocking comments.
(https://github.com/bitcoin/bitcoin/pull/30072#pullrequestreview-2061682943)
utACK [2fd34ba](https://github.com/bitcoin/bitcoin/pull/30072/commits/2fd34ba504957331f5a08614b6e1f8317260f04d)
Sorry it took me a while to get familiar with the validation logic. I left some non-blocking comments.
💬 sr-gi commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1603916879)
In 90b60a16731ac756dce44c29f57d111f4dffba75
nit (not-blocking):
```suggestion
/** Whether CPFP and RBF carveouts are granted. */
```
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1603916879)
In 90b60a16731ac756dce44c29f57d111f4dffba75
nit (not-blocking):
```suggestion
/** Whether CPFP and RBF carveouts are granted. */
```
💬 sr-gi commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1612127074)
Is there any case in which `m_allow_sibling_eviction` is not shadowing `m_allow_replacement`? All the uses I see, they have the same value, or if one is set, the other is assumed.
Is that just the case now, and may it be different in a follow-up? Otherwise, what is the point of having two flags?
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1612127074)
Is there any case in which `m_allow_sibling_eviction` is not shadowing `m_allow_replacement`? All the uses I see, they have the same value, or if one is set, the other is assumed.
Is that just the case now, and may it be different in a follow-up? Otherwise, what is the point of having two flags?
💬 sr-gi commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1603924191)
In 90b60a16731ac756dce44c29f57d111f4dffba75
I think this comment is not accurate (it was not all that accurate before that PR either). At this point, `CalculateMemPoolAncestors` has only been called once. The only case in which this caching applies is in the last else case of this context. I think this should be reworded
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1603924191)
In 90b60a16731ac756dce44c29f57d111f4dffba75
I think this comment is not accurate (it was not all that accurate before that PR either). At this point, `CalculateMemPoolAncestors` has only been called once. The only case in which this caching applies is in the last else case of this context. I think this should be reworded
💬 sr-gi commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1603949576)
In 90b60a16731ac756dce44c29f57d111f4dffba75
If we are removing `txns.size() > 1` because it is a truism, we should at least assume it to make sure we are not overlooking it
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1603949576)
In 90b60a16731ac756dce44c29f57d111f4dffba75
If we are removing `txns.size() > 1` because it is a truism, we should at least assume it to make sure we are not overlooking it
💬 sr-gi commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1603952684)
In 90b60a16731ac756dce44c29f57d111f4dffba75
I see `:%s/even/or` was applied a few lines above, but this suggestion seems not to have been covered. Was it applied to the above line by mistake?
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1603952684)
In 90b60a16731ac756dce44c29f57d111f4dffba75
I see `:%s/even/or` was applied a few lines above, but this suggestion seems not to have been covered. Was it applied to the above line by mistake?
💬 maflcko commented on issue "prune shall not delete blocks it did not download":
(https://github.com/bitcoin/bitcoin/issues/30163#issuecomment-2127810175)
Using the same blocksdir for two different nodes is not supported. Nodes may download blocks in a different order and save them to different locations in the blocksfiles. This will lead to an error at some point, latest when one of the nodes can't find a block where it believes to be one.
Currently, I don't think what you are trying to achieve is possible without copying blocks.
> If bitcoin-core assumes pruning to be enabled by default, at least it should have asked for confirmation to de
...
(https://github.com/bitcoin/bitcoin/issues/30163#issuecomment-2127810175)
Using the same blocksdir for two different nodes is not supported. Nodes may download blocks in a different order and save them to different locations in the blocksfiles. This will lead to an error at some point, latest when one of the nodes can't find a block where it believes to be one.
Currently, I don't think what you are trying to achieve is possible without copying blocks.
> If bitcoin-core assumes pruning to be enabled by default, at least it should have asked for confirmation to de
...