💬 maflcko commented on issue "Fuzzing related configuration/build options can be improved":
(https://github.com/bitcoin/bitcoin/issues/30318#issuecomment-2242390222)
> > > * `--enable-fuzz-binary=yes|no|exclusive`, but renaming it would break every fuzzing script. Maybe the transition to CMake is a good time to improve this since scripts would have to be adjusted anyway.
> >
> >
> > Seems fine to do, if `-DBUILD_FUZZ_BINARY=ON/OFF/EXCL...` is easy to implement.
>
> It's not a problem at all.

On thing that would be confusing, is that `EXCL...` (or whatever the third state is) would have to also enable `-DABORT_ON_FAILED_ASSUME`. At this point, I qu
...

💬 maflcko commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#issuecomment-2242444648)
> But fortunately I do not think we need to nitpick and argue about behavior the `Get*Arg` functions because **if any developer does not like the behavior of the `Get*Arg` functions they can call the `GetSetting` function instead** which gives access to the underlying, validated `UniValue` setting and allows any developers to implement any behavior they would like to implement to parse their settings using the UniValue API.

I looked at the feedback so far, and I wonder if it could make sense
...

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2242466297)
Checked that the code didn't change in the last push, only the commit message, which looked fine on a glance.

If you re-touch you can change "and scan past" to "and may scan past", or "and possibly scan past". Otherwise, it seems to imply this bug was actually hit.

Also, you can adjust the pull request description with a motivation and fix. Otherwise, it is just two random links, and reviewers will have a hard time seeing the motivation and gist of the changes.

💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#issuecomment-2242466411)

re-ACK 788fe9cc9ab979c5e14f544ae05dc46436892b81 🔃

<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 788fe9cc9ab979c5e
...

💬 maflcko commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2242491437)
> reduce CI load

Some more machines were added a few weeks back to deal with increased overall activity, so I think the load should be handleable now, unless someone pushes to 10 different pull requests every 20 minutes in a loop.

Looks like you fixed it in the meantime, but if CI load comes up again, you may create an issue so that it can be fixed.

💬 fanquake commented on pull request "depends: build zeromq with CMake":
(https://github.com/bitcoin/bitcoin/pull/29723#issuecomment-2242543346)
> CMake compiles 7 fewer source files compared to Autotools. It skips::

That's expected. We aren't opting in to either of these features.

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2242545417)
Update https://github.com/bitcoin/bitcoin/commit/b8b3a9f18670ec7bf246a57950cdae7e034a264d -> https://github.com/bitcoin/bitcoin/commit/56267cf84281789186035f09898cce3d749748ec ([apply-taptweak-method-04](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-04) -> [apply-taptweak-method-05](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-05) ([compare](https://github.com/josibake/bitcoin/compare/apply-taptweak-method-04..josibake:apply-taptweak-method-05))

Realised I u
...

💬 josibake commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1686283547)
Updated.

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1686287447)
The only way this _should_ fail is `coinbase_script` is invalid (e.g. too large). But that currently results in a `throw std::runtime_error` inside `BlockAssembler::CreateNewBlock`.

Changed the three lines to `CHECK_NONFATAL`.

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#discussion_r1686288232)
Done

💬 Sjors commented on pull request "Have createNewBlock() return a BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/30440#issuecomment-2242552923)
I split `getCoinbaseMerklePath()` and `submitSolution() out of this PR and moved them to [TODO].

Since it's astronomically unlikely multiprocess will be in the upcoming v28.0 release, we can add these functions later. They both use the new `BlockMerkleRoot` which I need to study in more detail myself.

Rebased and addressed comments above: https://github.com/bitcoin/bitcoin/pull/30440#pullrequestreview-2186655119

Marked as no longer draft.

👋 Sjors's pull request is ready for review: "Have createNewBlock() return a BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/30440)

💬 paplorinc commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#issuecomment-2242574270)
CI failure seems unrelated, please restart or rebase so we can ACK - and don't worry about our reacks :)

> Update https://github.com/bitcoin/bitcoin/commit/b8b3a9f18670ec7bf246a57950cdae7e034a264d -> https://github.com/bitcoin/bitcoin/commit/56267cf84281789186035f09898cce3d749748ec ([apply-taptweak-method-04](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-04) -> [apply-taptweak-method-05](https://github.com/josibake/bitcoin/tree/apply-taptweak-method-05) ([compare](https://git
...

💬 hebasto commented on pull request "locks: introduce mutex for tx download, flush rejection filters once per tip change":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1686303768)
The `PeerManagerImpl::ProcessMessages` implementation has multiple early `return` statements. In such a case, the Developer Notes [suggest](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization) to:
> Combine annotations in function declarations with run-time asserts in function definitions

i.e., `AssertLockNotHeld(m_tx_download_mutex);`

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2242578323)
ACK a512245e217199208807214b09bdb256362d08af

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686306726)
Sure, but you did change it, so unless there's any particular reason, I think the original order makes more sense

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686309888)
I understood, that's why I recommended the name change. We don't say `trim` is fragile, just because more values become valid after they're trimmed, we call it lenient, right?

💬 paplorinc commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686312388)
The reason is that it's not a setter, but a complex method that changes the internal state.

💬 stickies-v commented on pull request "rest: Reject truncated hex txid early in getutxos parsing":
(https://github.com/bitcoin/bitcoin/pull/30482#discussion_r1686313096)
I don't see the point of bloating the build logs with this, a tracking issue seems more appropriate if we're worried it'll be forgotten.

💬 hebasto commented on pull request "locks: introduce mutex for tx download, flush rejection filters once per tip change":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1686314984)
Do we need to hold `m_tx_download_mutex` for this call: https://github.com/bitcoin/bitcoin/blob/c85accecafc20f6a6ae94bdf6cdd3ba9747218fd/src/net_processing.cpp#L6327-L6328 a few lines down?