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

💬 darosior commented on pull request "fuzz: Deglobalize signature cache in sigcache test":
(https://github.com/bitcoin/bitcoin/pull/30447#discussion_r1686316001)
nit: this would invalidate existing seeds.

⚠️ Sjors opened an issue: "Spurious markdown linter failure"
(https://github.com/bitcoin/bitcoin/issues/30496)
The linter job failed for https://github.com/bitcoin/bitcoin/pull/30440

```
One or more markdown links are broken.

Relative links are preferred (but not required) as jumping to file works natively within Emacs.

Markdown link errors found:
[Err ] ./pyenv/README.md (12, 1) => /terminal_output.png - Target filename not found.


^---- ⚠️ Failure generated from lint check 'markdown'!
Check that markdown links resolve
```

The PR does not touch any markdown files.

...

💬 maflcko commented on issue "Spurious markdown linter failure":
(https://github.com/bitcoin/bitcoin/issues/30496#issuecomment-2242612557)
I guess the folder is dirty after building python (after the cache got evicted randomly).

So possibly a solution would be to clean the folder after building python.