💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612014056)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
Not important, but since these arguments are being inserted in a map, it would probably be a little better to pass them by value instead of reference, and use std::move so they can be moved instead of copied.
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612014056)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
Not important, but since these arguments are being inserted in a map, it would probably be a little better to pass them by value instead of reference, and use std::move so they can be moved instead of copied.
💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612033198)
In commit "node: update uiInterface whenever warnings updated" (396b261fc72d47d3ac4420d2db9910197a21d5ab)
Can commit message be updated to say what this change in behavior here is? It looks like now this UI notification will be triggered if a fatal error happens, so not sure if that is good or bad. It might be possible to avoid this by moving the uiInterface call to node/kernel_notifications.cpp instead. That would be good for consistency since uiInterface is already accessed there, and it wo
...
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1612033198)
In commit "node: update uiInterface whenever warnings updated" (396b261fc72d47d3ac4420d2db9910197a21d5ab)
Can commit message be updated to say what this change in behavior here is? It looks like now this UI notification will be triggered if a fatal error happens, so not sure if that is good or bad. It might be possible to avoid this by moving the uiInterface call to node/kernel_notifications.cpp instead. That would be good for consistency since uiInterface is already accessed there, and it wo
...
💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611957425)
In commit "move-only: move warnings from common to node" (6681395632c0ab5e048b34d63d26636856d4202d)
s/RPc/RPC/ in commit message
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611957425)
In commit "move-only: move warnings from common to node" (6681395632c0ab5e048b34d63d26636856d4202d)
s/RPc/RPC/ in commit message
💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611964045)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
Would be good to have release notes for this change mentioning new -alertnotify behavior, since it now can be trigged multiple times and in new cases
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611964045)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
Would be good to have release notes for this change mentioning new -alertnotify behavior, since it now can be trigged multiple times and in new cases
💬 ryanofsky commented on pull request "Encapsulate warnings in generalized node::Warnings and remove globals":
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611973476)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
I guess this change also affects the order of warnings. It seems like previous code was written to consistently put the pre-release warning first, followed by the miscellaneous warnings, the large-work chain warning, and the time offset warning last.
Changing this is probably fine, but it would be good to note the change in the commit message. Could also note in setWarning d
...
(https://github.com/bitcoin/bitcoin/pull/30058#discussion_r1611973476)
In commit "introduce and use the generalized `node::Warnings` interface" (6152c20970c4c4db308879524d4d1c999c4580ae)
I guess this change also affects the order of warnings. It seems like previous code was written to consistently put the pre-release warning first, followed by the miscellaneous warnings, the large-work chain warning, and the time offset warning last.
Changing this is probably fine, but it would be good to note the change in the commit message. Could also note in setWarning d
...
💬 luke-jr commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1612059228)
Is it a safe assumption that this cannot happen except an interrupted replay?
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1612059228)
Is it a safe assumption that this cannot happen except an interrupted replay?
💬 luke-jr commented on pull request "Fee Estimation via Fee rate Forecasters":
(https://github.com/bitcoin/bitcoin/pull/30157#issuecomment-2127682430)
>Make the fee estimator aware of the state of the mempool, allowing it to respond to changing conditions immediately.
The state of the node's mempool may not accurately reflect the state of others' mempools, and not even its own mempool when the block is found in the future. It isn't a good single source of information. Perhaps it is a good idea to use it as a secondary source, but probably it should only ever adjust fee estimations upward, not down.
(https://github.com/bitcoin/bitcoin/pull/30157#issuecomment-2127682430)
>Make the fee estimator aware of the state of the mempool, allowing it to respond to changing conditions immediately.
The state of the node's mempool may not accurately reflect the state of others' mempools, and not even its own mempool when the block is found in the future. It isn't a good single source of information. Perhaps it is a good idea to use it as a secondary source, but probably it should only ever adjust fee estimations upward, not down.
📝 sipa opened a pull request: "util: add RingBuffer"
(https://github.com/bitcoin/bitcoin/pull/30161)
Extracted from #30126.
This adds a `RingBuffer` data type, inspired by `std::deque`, but backed by a single allocated memory region used as a ring buffer instead of a linked list of arrays. This gives better memory locality and less allocation overhead, plus better guarantees (some C++ standard library implementations, though not libstdc++ and libc++, use a separate allocation per element in a deque).
It is intended for the candidate set search queue in #30126, but may be useful as a repla
...
(https://github.com/bitcoin/bitcoin/pull/30161)
Extracted from #30126.
This adds a `RingBuffer` data type, inspired by `std::deque`, but backed by a single allocated memory region used as a ring buffer instead of a linked list of arrays. This gives better memory locality and less allocation overhead, plus better guarantees (some C++ standard library implementations, though not libstdc++ and libc++, use a separate allocation per element in a deque).
It is intended for the candidate set search queue in #30126, but may be useful as a repla
...
💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1612065514)
not sure about unconditionally crashing the node on any failure here. This may or may not be an issue, so the call site will have to decide about this.
For example, If an RPC user dumps the mempool to an external (corrupt) USB thumb drive, a failure shouldn't crash the node, but notify the user over the RPC.
Also, I am not sure about involving logging in this low-level code. Logging may not be set up, so this may never be printed before the crash.
Also, I am not sure about the log messa
...
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1612065514)
not sure about unconditionally crashing the node on any failure here. This may or may not be an issue, so the call site will have to decide about this.
For example, If an RPC user dumps the mempool to an external (corrupt) USB thumb drive, a failure shouldn't crash the node, but notify the user over the RPC.
Also, I am not sure about involving logging in this low-level code. Logging may not be set up, so this may never be printed before the crash.
Also, I am not sure about the log messa
...
💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1612068360)
Oh, now I see this is required, because otherwise the node would crash on this error instead of continuing with the current code, if this was missing?
Not sure this is desirable, see also https://github.com/bitcoin/bitcoin/pull/29307/files#r1612065514
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1612068360)
Oh, now I see this is required, because otherwise the node would crash on this error instead of continuing with the current code, if this was missing?
Not sure this is desirable, see also https://github.com/bitcoin/bitcoin/pull/29307/files#r1612065514
💬 murchandamus commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1612056701)
I thought sibling eviction was relevant in a TRUC Transaction constellation when there already exist an unconfirmed parent and unconfirmed child, and now a newly submitted transaction is a child of the same parent, but spends a different output than the original child, i.e. not actually in conflict with the original child except for the topological restrictions affecting TRUC Transactions.
If I got all that right, I would posit that "This only occurs in single transaction package settings." i
...
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1612056701)
I thought sibling eviction was relevant in a TRUC Transaction constellation when there already exist an unconfirmed parent and unconfirmed child, and now a newly submitted transaction is a child of the same parent, but spends a different output than the original child, i.e. not actually in conflict with the original child except for the topological restrictions affecting TRUC Transactions.
If I got all that right, I would posit that "This only occurs in single transaction package settings." i
...
💬 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
...