💬 petertodd commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2237833642)
> @petertodd I'll give a non-TRUC version another try and report back. I may have been over-thinking things. If it works out it will be very similar in code structure...
>
> edit: oh of course I already noted in OP: We don't allow individual transactions with 0-fee outside of TRUC, for now due to trimming computation concerns. Cluster mempool does away with this restriction entirely. I'll rework the code to upgrade to non-TRUC usage once cluster mempool hits.
I think the worthwhile questio
...
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2237833642)
> @petertodd I'll give a non-TRUC version another try and report back. I may have been over-thinking things. If it works out it will be very similar in code structure...
>
> edit: oh of course I already noted in OP: We don't allow individual transactions with 0-fee outside of TRUC, for now due to trimming computation concerns. Cluster mempool does away with this restriction entirely. I'll rework the code to upgrade to non-TRUC usage once cluster mempool hits.
I think the worthwhile questio
...
💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683669595)
The other things can be safely set to false, so we can be sure they're false without an assertion. But if you've set a callback you've also gotten an `iterator` that you might intend to pass to `DeleteCallback` later, which would then crash if we had cleared `m_print_callbacks` here.
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683669595)
The other things can be safely set to false, so we can be sure they're false without an assertion. But if you've set a callback you've also gotten an `iterator` that you might intend to pass to `DeleteCallback` later, which would then crash if we had cleared `m_print_callbacks` here.
🤔 ryanofsky reviewed a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2187172873)
re: https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2185910679
> > but internally it is calling the uint256S function which expects a nul-terminated string
>
> Maybe I'm being too pedantic (or wrong), but I'm not sure this is 100% correct? I don't think the `uint256S(std::string_view str)` `str` needs to be null-terminated.
Yes, both explanations are saying the same thing but mine is unnecessarily confusing. The `uint256S` function "expects" a nul-terminated string becau
...
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2187172873)
re: https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2185910679
> > but internally it is calling the uint256S function which expects a nul-terminated string
>
> Maybe I'm being too pedantic (or wrong), but I'm not sure this is 100% correct? I don't think the `uint256S(std::string_view str)` `str` needs to be null-terminated.
Yes, both explanations are saying the same thing but mine is unnecessarily confusing. The `uint256S` function "expects" a nul-terminated string becau
...
💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683689226)
re: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682892102
> review note: it seems the `while` loop was replaced with `for` loop purely for readability, I don't see any functional change here.
To be sure, replacing the previous while loop is an important part of the fix. If the previous while loop was kept, it could iterate past the end of the of `string_view` and read uninitialized memory. This for loop (and the while loop I suggested in https://github.com/bitcoin/bitcoin/pu
...
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683689226)
re: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682892102
> review note: it seems the `while` loop was replaced with `for` loop purely for readability, I don't see any functional change here.
To be sure, replacing the previous while loop is an important part of the fix. If the previous while loop was kept, it could iterate past the end of the of `string_view` and read uninitialized memory. This for loop (and the while loop I suggested in https://github.com/bitcoin/bitcoin/pu
...
💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742290)
Added a `[...]` marker, since it was easy.
In order to trigger it you'd need to have 1MB of log message on a single, incomplete line, which seems a bit ridiculous. Long-term, I think it would make more sense to stop intentionally doing incomplete lines (since that introduces a race condition if other threads are also logging), and just add a newline if it's missing... *shrug*
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742290)
Added a `[...]` marker, since it was easy.
In order to trigger it you'd need to have 1MB of log message on a single, incomplete line, which seems a bit ridiculous. Long-term, I think it would make more sense to stop intentionally doing incomplete lines (since that introduces a race condition if other threads are also logging), and just add a newline if it's missing... *shrug*
💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742446)
Changed to strprintf. Got a bit target fixated there...
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742446)
Changed to strprintf. Got a bit target fixated there...
💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742656)
That's a bug, changed.
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742656)
That's a bug, changed.
💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742718)
Done
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742718)
Done
💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683745201)
I'd like to change `Enabled()` and `WillLogCategoryLevel()` functions to be lockless so that calls to `Log*()` don't block when they have nothing to do. I think the above changes would mesh well with that.
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683745201)
I'd like to change `Enabled()` and `WillLogCategoryLevel()` functions to be lockless so that calls to `Log*()` don't block when they have nothing to do. I think the above changes would mesh well with that.
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683805668)
Fair enough, but I am mostly thinking what would be the use case for possibly confusing code like `m_print_to_console=true;DisableLogging()`? Given that this function also `assert(m_buffering)`, the code can only be called when logging was never active, I'd still say it makes sense to at least `Assume(!m_print_to*)`.
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683805668)
Fair enough, but I am mostly thinking what would be the use case for possibly confusing code like `m_print_to_console=true;DisableLogging()`? Given that this function also `assert(m_buffering)`, the code can only be called when logging was never active, I'd still say it makes sense to at least `Assume(!m_print_to*)`.
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683814101)
nit in 1cd8386a18db0ced6da0c5f42e952e81fa02867c:
```
clang-tidy-18 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/logging.cpp
logging.cpp:19:13: error: using decl 'ToString' is unused [misc-unused-using-decls,-warnings-as-errors]
19 | using util::ToString;
| ^
logging.cpp:19:13: note: remove the using
19 | using util::ToString;
...
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683814101)
nit in 1cd8386a18db0ced6da0c5f42e952e81fa02867c:
```
clang-tidy-18 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/logging.cpp
logging.cpp:19:13: error: using decl 'ToString' is unused [misc-unused-using-decls,-warnings-as-errors]
19 | using util::ToString;
| ^
logging.cpp:19:13: note: remove the using
19 | using util::ToString;
...
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683814416)
nit in https://github.com/bitcoin/bitcoin/commit/1cd8386a18db0ced6da0c5f42e952e81fa02867c:
Could use `RemovePrefixView` for consistency and to avoid a useless string constructor?
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683814416)
nit in https://github.com/bitcoin/bitcoin/commit/1cd8386a18db0ced6da0c5f42e952e81fa02867c:
Could use `RemovePrefixView` for consistency and to avoid a useless string constructor?
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#issuecomment-2238236358)
lgtm, after a CI fix.
re-ACK 1cd8386a18db0ced6da0c5f42e952e81fa02867c 🏡
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: r
...
(https://github.com/bitcoin/bitcoin/pull/30386#issuecomment-2238236358)
lgtm, after a CI fix.
re-ACK 1cd8386a18db0ced6da0c5f42e952e81fa02867c 🏡
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: r
...
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2238241424)
> > are lacking a good description that actually says what the bug is.
>
> I agree, I spent quite a bit of time getting to the bottom of this and more documentation would have sped that up
I agree as well. Thanks for brining it up! This should be fixed before merge, other than that, I'd say it is ready.
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2238241424)
> > are lacking a good description that actually says what the bug is.
>
> I agree, I spent quite a bit of time getting to the bottom of this and more documentation would have sped that up
I agree as well. Thanks for brining it up! This should be fixed before merge, other than that, I'd say it is ready.
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683831519)
> * So if there is a space in the string, or a 1 replaced with an I, it is treated like the end of input, and the resulting blob may have a combination of new and old values.
I don't think this is true. All "old" values will zeroed in the first line of `SetHex`.
The other shortcomings (no length check and no hex check) are true.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683831519)
> * So if there is a space in the string, or a 1 replaced with an I, it is treated like the end of input, and the resulting blob may have a combination of new and old values.
I don't think this is true. All "old" values will zeroed in the first line of `SetHex`.
The other shortcomings (no length check and no hex check) are true.
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2238277781)
Retested the speed gains after the recent minor changes, with a single run per commit:
> Summary
```python
'echo d7f94e && ./src/bitcoind -datadir=/mnt/hdd1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=400000 -printtoconsole=0' ran
1.12 times faster than 'echo 0383de && ./src/bitcoind -datadir=/mnt/hdd1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=400000 -printtoconsole=0'
```
i.e. 12% faster for the first 400000 blocks after the chang
...
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2238277781)
Retested the speed gains after the recent minor changes, with a single run per commit:
> Summary
```python
'echo d7f94e && ./src/bitcoind -datadir=/mnt/hdd1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=400000 -printtoconsole=0' ran
1.12 times faster than 'echo 0383de && ./src/bitcoind -datadir=/mnt/hdd1/BitcoinData -dbcache=16384 -prune=550 -stopatheight=400000 -printtoconsole=0'
```
i.e. 12% faster for the first 400000 blocks after the chang
...
📝 maflcko opened a pull request: " rest: Reject truncated hex txid early in getutxos parsing "
(https://github.com/bitcoin/bitcoin/pull/30482)
In `rest_getutxos` truncated txids such as `aa` or `ff` are accepted. This is brittle at best.
Fix it by rejecting any truncated (or overlarge) input.
----
Review note: This also starts a major refactor to rework hex parsing in Bitcoin Core, meaning that a few refactor commits are included as well. They are explained individually in the commit message and the work will be continued in the future.
(https://github.com/bitcoin/bitcoin/pull/30482)
In `rest_getutxos` truncated txids such as `aa` or `ff` are accepted. This is brittle at best.
Fix it by rejecting any truncated (or overlarge) input.
----
Review note: This also starts a major refactor to rework hex parsing in Bitcoin Core, meaning that a few refactor commits are included as well. They are explained individually in the commit message and the work will be continued in the future.
⚠️ Sjors opened an issue: "guix: build multiprocess"
(https://github.com/bitcoin/bitcoin/issues/30483)
In order to run a Stratum v2 Template Provider as a "sidecar" via IPC, and to have `bitcoin-node` be reproducibly built, libmultiprocess needs to be built with guix.
Presumably this would happen as part of #10102 or #30437 eventually.
However if the TP "sidecar" is built on top of the Bitcoin Core codebase, like https://github.com/Sjors/bitcoin/pull/48 attempts, then _that_ binary should also be Guix built. So it makes sense to track this independently.
I made a naive attempt in https:/
...
(https://github.com/bitcoin/bitcoin/issues/30483)
In order to run a Stratum v2 Template Provider as a "sidecar" via IPC, and to have `bitcoin-node` be reproducibly built, libmultiprocess needs to be built with guix.
Presumably this would happen as part of #10102 or #30437 eventually.
However if the TP "sidecar" is built on top of the Bitcoin Core codebase, like https://github.com/Sjors/bitcoin/pull/48 attempts, then _that_ binary should also be Guix built. So it makes sense to track this independently.
I made a naive attempt in https:/
...
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683934569)
A partial fix is in https://github.com/bitcoin/bitcoin/pull/30482. It will be continued in the future.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683934569)
A partial fix is in https://github.com/bitcoin/bitcoin/pull/30482. It will be continued in the future.
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683934828)
> Yes, those are known shortcomings. The function will be removed completely in the future, but this will be done in a follow-up.
A partial fix is in https://github.com/bitcoin/bitcoin/pull/30482. It will be continued in the future.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683934828)
> Yes, those are known shortcomings. The function will be removed completely in the future, but this will be done in a follow-up.
A partial fix is in https://github.com/bitcoin/bitcoin/pull/30482. It will be continued in the future.