💬 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.

💬 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.

💬 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

💬 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
...

💬 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.

💬 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

⚠️ 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
...

💬 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)

💬 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`)

💬 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
...

💬 TheCharlatan commented on pull request "fuzz: Deglobalize signature cache in sigcache test":
(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).

💬 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.

💬 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

👍 dergoegge approved a pull request: "fuzz: Deglobalize signature cache in sigcache test"
(https://github.com/bitcoin/bitcoin/pull/30447#pullrequestreview-2188536419)
utACK fae0db0360466aed536f4ce96d357cf579100080

📝 Sjors opened a pull request: "ci: skip Github CI on branch pushes for forks"
(https://github.com/bitcoin/bitcoin/pull/30487)
Similar to e9bfbb5414ab14ca14d8edcfdf77f28c9ed67c33 for Cirrus, but not opt-in, because Github CI lacks custom variables.

This was was originally part of #29274, but left out in order for that PR to be entirely about Cirrus CI. It was suggested as a followup in https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2214759087.

I've been using this commit in #29432 (on this repo) and https://github.com/Sjors/bitcoin/pull/48 (my fork).

💬 Sjors commented on pull request "Fix issues with CI on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2239482323)
Picked @ryanofsky's first suggestion into #30487.

So far ARM jobs have not been a source of headaches on my own fork (only TSAN and MSAN are atm).

💬 t-bast 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-2239486786)
> 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

Right, we discussed that in this thread, but the responses to that thread show that it's not a design oversight, using a single anchor has drawbacks as well. But it's quite off-topic for this PR though.

👋 fanquake's pull request is ready for review: "guix: bump time-machine to 1aa8dfaeec3c6e4e587aadf7440246f7c5c04b9f"
(https://github.com/bitcoin/bitcoin/pull/30452)

💬 fanquake commented on pull request "guix: bump time-machine to 1aa8dfaeec3c6e4e587aadf7440246f7c5c04b9f":
(https://github.com/bitcoin/bitcoin/pull/30452#issuecomment-2239495817)
Fixed the cross-arch non-determinism by patching the store patch out of winpthreads.