💬 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.
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683935476)
> Well, it will be removed, given that someone writes the code and reviews it.
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_r1683935476)
> Well, it will be removed, given that someone writes the code and reviews it.
A partial fix is in https://github.com/bitcoin/bitcoin/pull/30482. It will be continued in the future.
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#issuecomment-2238562006)
re-ACK b4dd7ab43e8cfc2c171f67588e4e1ec2705393c2 🕴
<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: re-ACK b4dd7ab43e8cfc2c171f
...
(https://github.com/bitcoin/bitcoin/pull/30386#issuecomment-2238562006)
re-ACK b4dd7ab43e8cfc2c171f67588e4e1ec2705393c2 🕴
<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: re-ACK b4dd7ab43e8cfc2c171f
...
💬 stickies-v commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683976467)
> it could iterate past the end of the of `string_view` and read uninitialized memory.
Right, because on master we rely on stopping when `HexDigit()` returns a `-1`, which is fine when the string is null-terminated but is not safe otherwise. Thanks for pointing this out, this also clarifies my misunderstanding about your [earlier comment](https://github.com/bitcoin/bitcoin/pull/30436/#pullrequestreview-2186195988).
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683976467)
> it could iterate past the end of the of `string_view` and read uninitialized memory.
Right, because on master we rely on stopping when `HexDigit()` returns a `-1`, which is fine when the string is null-terminated but is not safe otherwise. Thanks for pointing this out, this also clarifies my misunderstanding about your [earlier comment](https://github.com/bitcoin/bitcoin/pull/30436/#pullrequestreview-2186195988).
⚠️ Arroz01 opened an issue: "New"
(https://github.com/bitcoin/bitcoin/issues/30484)
(https://github.com/bitcoin/bitcoin/issues/30484)
💬 maflcko commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1683988688)
> In other places, `os.path.realpath(__file__)` is used in conjunction with relative paths in the `test/functional/` directory.
Thank you for the explanation. It makes sense, given that cmake creates all links recursively in this dir.
However, this leads back to my original question: Why not change the other affected places?
This still fails for me (and I expect it to fail in the cmake branch as well):
```
$ ln $PWD/../test/functional/interface_rest.py ./test/functional/
$
...
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1683988688)
> In other places, `os.path.realpath(__file__)` is used in conjunction with relative paths in the `test/functional/` directory.
Thank you for the explanation. It makes sense, given that cmake creates all links recursively in this dir.
However, this leads back to my original question: Why not change the other affected places?
This still fails for me (and I expect it to fail in the cmake branch as well):
```
$ ln $PWD/../test/functional/interface_rest.py ./test/functional/
$
...
✅ maflcko closed an issue: "New"
(https://github.com/bitcoin/bitcoin/issues/30484)
(https://github.com/bitcoin/bitcoin/issues/30484)