💬 dergoegge commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#discussion_r1684377201)
Perhaps we move this check down to right before the descriptor related code? Afaict, there's no need to restrict the testing of the json parsing to 10k bytes.
As a side note, this test does too many things at once in my opinion and should probably be split up.
(https://github.com/bitcoin/bitcoin/pull/30473#discussion_r1684377201)
Perhaps we move this check down to right before the descriptor related code? Afaict, there's no need to restrict the testing of the json parsing to 10k bytes.
As a side note, this test does too many things at once in my opinion and should probably be split up.
💬 maflcko commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1684380835)
> 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_
Fixed in https://github.com/bitcoin/bitcoin/pull/30485 (but not yet removing the `m_started_new_line` to avoid a merge conflict here). I'll do it in a follow-up, after this one is merged, or if I get the OK to do it before this one is merged.
In any case, according to the cove
...
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1684380835)
> 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_
Fixed in https://github.com/bitcoin/bitcoin/pull/30485 (but not yet removing the `m_started_new_line` to avoid a merge conflict here). I'll do it in a follow-up, after this one is merged, or if I get the OK to do it before this one is merged.
In any case, according to the cove
...
💬 dergoegge commented on pull request "fuzz: Deglobalize signature cache in sigcache test":
(https://github.com/bitcoin/bitcoin/pull/30447#discussion_r1684384876)
```suggestion
FUZZ_TARGET(script_sigcache, .init = initialize_script_sigcache)
```
(https://github.com/bitcoin/bitcoin/pull/30447#discussion_r1684384876)
```suggestion
FUZZ_TARGET(script_sigcache, .init = initialize_script_sigcache)
```
💬 maflcko commented on pull request "fuzz: Limit parse_univalue input length":
(https://github.com/bitcoin/bitcoin/pull/30473#discussion_r1684401200)
> Perhaps we move this check down to right before the descriptor related code? Afaict, there's no need to restrict the testing of the json parsing to 10k bytes.
Good idea. Done!
(https://github.com/bitcoin/bitcoin/pull/30473#discussion_r1684401200)
> Perhaps we move this check down to right before the descriptor related code? Afaict, there's no need to restrict the testing of the json parsing to 10k bytes.
Good idea. Done!
👍 dergoegge approved a pull request: "fuzz: Limit parse_univalue input length"
(https://github.com/bitcoin/bitcoin/pull/30473#pullrequestreview-2188274893)
utACK fa80b16b20dffcb85b80f75fee64ed333f2062f9
(https://github.com/bitcoin/bitcoin/pull/30473#pullrequestreview-2188274893)
utACK fa80b16b20dffcb85b80f75fee64ed333f2062f9
💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2239221514)
Rebased, and fixed an issue with the most recent push. Guix build (aarch64):
```bash
c7aa6bd428ba4ea1c925dafe4df3505ad92e84a196b17c8cb7965d7db231e6a0 guix-build-0388ad0d65b6/output/aarch64-linux-gnu/SHA256SUMS.part
944e734a719886ec26aff12a80be67d28f2c4b7781a341894d3e2ca8477e3497 guix-build-0388ad0d65b6/output/aarch64-linux-gnu/bitcoin-0388ad0d65b6-aarch64-linux-gnu-debug.tar.gz
087ac7a0c1d87a95adacb9fa138aced37a172d4d5199fc75c64831d9f6211972 guix-build-0388ad0d65b6/output/aarch64-linux-gn
...
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2239221514)
Rebased, and fixed an issue with the most recent push. Guix build (aarch64):
```bash
c7aa6bd428ba4ea1c925dafe4df3505ad92e84a196b17c8cb7965d7db231e6a0 guix-build-0388ad0d65b6/output/aarch64-linux-gnu/SHA256SUMS.part
944e734a719886ec26aff12a80be67d28f2c4b7781a341894d3e2ca8477e3497 guix-build-0388ad0d65b6/output/aarch64-linux-gnu/bitcoin-0388ad0d65b6-aarch64-linux-gnu-debug.tar.gz
087ac7a0c1d87a95adacb9fa138aced37a172d4d5199fc75c64831d9f6211972 guix-build-0388ad0d65b6/output/aarch64-linux-gn
...
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684414871)
On a second thought, I think the `SetHex` is preferable in this case, because the code using `std::optional` requires more typing (minimally more) when parsing and when unwrapping. Given the `[[nodiscard]]` approach, both should be equally safe, so I picked the approach using less code.
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684414871)
On a second thought, I think the `SetHex` is preferable in this case, because the code using `std::optional` requires more typing (minimally more) when parsing and when unwrapping. Given the `[[nodiscard]]` approach, both should be equally safe, so I picked the approach using less code.
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684428613)
Thanks, but I'll leave it as-is for now.
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684428613)
Thanks, but I'll leave it as-is for now.
💬 maflcko commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684429034)
Thanks, edited commit message
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1684429034)
Thanks, edited commit message
💬 stickies-v commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2239279118)
Side note: we can use [bugprone-suspicious-stringview-data-usage](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/suspicious-stringview-data-usage.html) (from unreleased clang-19) to catch this going forward, when we have clang-19. I tried running clang-tidy-19 locally from master branch for longer than I probably should have, and eventually gave up.
As a poor man's alternative, I looked for other stringview data misusage based on some rough grepping, and couldn't find anything, but t
...
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2239279118)
Side note: we can use [bugprone-suspicious-stringview-data-usage](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/suspicious-stringview-data-usage.html) (from unreleased clang-19) to catch this going forward, when we have clang-19. I tried running clang-tidy-19 locally from master branch for longer than I probably should have, and eventually gave up.
As a poor man's alternative, I looked for other stringview data misusage based on some rough grepping, and couldn't find anything, but t
...
💬 maflcko commented on pull request "log: Remove NOLINT(bitcoin-unterminated-logprintf)":
(https://github.com/bitcoin/bitcoin/pull/30485#discussion_r1684451445)
Arguably, this is a "bugfix" to add a missing `\n` in the unlikely case where the buffer is exactly filled and the last character is overwritten from `\n` to `\0`.
However, I am not sure if anyone ever ran into this logging bug, so I am just leaving a comment here.
(https://github.com/bitcoin/bitcoin/pull/30485#discussion_r1684451445)
Arguably, this is a "bugfix" to add a missing `\n` in the unlikely case where the buffer is exactly filled and the last character is overwritten from `\n` to `\0`.
However, I am not sure if anyone ever ran into this logging bug, so I am just leaving a comment here.
💬 maflcko commented on issue "intermittent issue in wallet_backwards_compatibility.py: line 245, in run_test assert txs[3]["abandoned"] AssertionError":
(https://github.com/bitcoin/bitcoin/issues/29806#issuecomment-2239311824)
https://cirrus-ci.com/task/5052912852795392?logs=ci#L3260
(https://github.com/bitcoin/bitcoin/issues/29806#issuecomment-2239311824)
https://cirrus-ci.com/task/5052912852795392?logs=ci#L3260
⚠️ ajtowns opened an issue: "Logging controls"
(https://github.com/bitcoin/bitcoin/issues/30486)
### Please describe the feature you'd like to see added.
Categories in the logging system currently work as follows:
* You can set a global log level (defaulting to info), to enable debug or trace logs in all categories (`-loglevel=trace`)
* You can override the global log level on a per-category basis, either making it more verbose (override info to debug or trace, or override debug to trace), or to make it less verbose (override a global default of debug or trace to a category specific
...
(https://github.com/bitcoin/bitcoin/issues/30486)
### Please describe the feature you'd like to see added.
Categories in the logging system currently work as follows:
* You can set a global log level (defaulting to info), to enable debug or trace logs in all categories (`-loglevel=trace`)
* You can override the global log level on a per-category basis, either making it more verbose (override info to debug or trace, or override debug to trace), or to make it less verbose (override a global default of debug or trace to a category specific
...
💬 maflcko commented on pull request "test: Fix intermittent issue in wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/29982#issuecomment-2239327795)
The real fix would be https://github.com/bitcoin/bitcoin/pull/18840 (see also the incoming links on that pull)
(https://github.com/bitcoin/bitcoin/pull/29982#issuecomment-2239327795)
The real fix would be https://github.com/bitcoin/bitcoin/pull/18840 (see also the incoming links on that pull)
💬 maflcko commented on pull request "log: Remove NOLINT(bitcoin-unterminated-logprintf)":
(https://github.com/bitcoin/bitcoin/pull/30485#discussion_r1684495357)
(It is possible to test this "bugfix" by reducing both buffer sizes sufficiently and then running with `-debug=leveldb -printtoconsole`)
(https://github.com/bitcoin/bitcoin/pull/30485#discussion_r1684495357)
(It is possible to test this "bugfix" by reducing both buffer sizes sufficiently and then running with `-debug=leveldb -printtoconsole`)
💬 petertodd commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2239410595)
I'm also going to point out that restricting ephemeral dust transactions to zero fee seems to create protocol design problems around HTLCs: https://delvingbitcoin.org/t/ephemeral-anchors-and-mevil/383
The simplest solution with dust HTLCs is to just put the value towards mining fees. Forcing them to be put towards the keyless ephemeral output potentially creates incentive problems around stealing that output. If ephemeral dust transactions have fewer rules around them, that kind of design pro
...
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2239410595)
I'm also going to point out that restricting ephemeral dust transactions to zero fee seems to create protocol design problems around HTLCs: https://delvingbitcoin.org/t/ephemeral-anchors-and-mevil/383
The simplest solution with dust HTLCs is to just put the value towards mining fees. Forcing them to be put towards the keyless ephemeral output potentially creates incentive problems around stealing that output. If ephemeral dust transactions have fewer rules around them, that kind of design pro
...
💬 TheCharlatan commented on pull request "fuzz: Deglobalize signature cache in sigcache test":
(https://github.com/bitcoin/bitcoin/pull/30447#discussion_r1684533562)
damn...
(https://github.com/bitcoin/bitcoin/pull/30447#discussion_r1684533562)
damn...
💬 Sjors commented on pull request "Stratum v2 Template Provider common functionality":
(https://github.com/bitcoin/bitcoin/pull/30475#issuecomment-2239442174)
I'm looking into moving #30332 and this PR to my fork so reduce the stack a bit. However I keep getting seemingly spurious MSAN / TSAN failures which I've never seen here. So I'll need to iron out some bugs in self-hosted CI first.
See e.g. https://cirrus-ci.com/task/5819158305177600 for https://github.com/Sjors/bitcoin/pull/50 which is identical to #30332 (which passed CI).
(https://github.com/bitcoin/bitcoin/pull/30475#issuecomment-2239442174)
I'm looking into moving #30332 and this PR to my fork so reduce the stack a bit. However I keep getting seemingly spurious MSAN / TSAN failures which I've never seen here. So I'll need to iron out some bugs in self-hosted CI first.
See e.g. https://cirrus-ci.com/task/5819158305177600 for https://github.com/Sjors/bitcoin/pull/50 which is identical to #30332 (which passed CI).
💬 TheCharlatan commented on pull request "fuzz: Deglobalize signature cache in sigcache test":
(https://github.com/bitcoin/bitcoin/pull/30447#issuecomment-2239444854)
Updated ae2a4c8f6e0b44cd69b057ae8bc3d542786413e4 -> fae0db0360466aed536f4ce96d357cf579100080 ([fuzzScriptCache_1](https://github.com/TheCharlatan/bitcoin/tree/fuzzScriptCache_1) -> [fuzzScriptCache_2](https://github.com/TheCharlatan/bitcoin/tree/fuzzScriptCache_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/fuzzScriptCache_1..fuzzScriptCache_2))
* Applied @dergoegge's [suggestion](https://github.com/bitcoin/bitcoin/pull/30447#discussion_r1684384876), fixing call to init.
(https://github.com/bitcoin/bitcoin/pull/30447#issuecomment-2239444854)
Updated ae2a4c8f6e0b44cd69b057ae8bc3d542786413e4 -> fae0db0360466aed536f4ce96d357cf579100080 ([fuzzScriptCache_1](https://github.com/TheCharlatan/bitcoin/tree/fuzzScriptCache_1) -> [fuzzScriptCache_2](https://github.com/TheCharlatan/bitcoin/tree/fuzzScriptCache_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/fuzzScriptCache_1..fuzzScriptCache_2))
* Applied @dergoegge's [suggestion](https://github.com/bitcoin/bitcoin/pull/30447#discussion_r1684384876), fixing call to init.
💬 petertodd commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2239445447)
> since we'll always have a single anchor
FYI anchor channels having two anchor outputs is actually a design oversight. Only one anchor output is needed in almost all cases: https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-December/004246.html
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2239445447)
> since we'll always have a single anchor
FYI anchor channels having two anchor outputs is actually a design oversight. Only one anchor output is needed in almost all cases: https://lists.linuxfoundation.org/pipermail/lightning-dev/2023-December/004246.html