💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1616695926)
done.

💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1616701338)
> I still think this can be simplified. initiate_v2_handshake is going to be called, asynchronously, when creating this object. You can reduce a lot of the boilerplate, and get rid of magic_sent, as long as you do the last bit manually on the test (or just create another method, like continue_v2_handshake).

yes, that can be done since the last bit is being sent from `MainThread` anyways.

before:
1. send both 4 bytes network magic (first 4 bytes of ellswift) - `NetworkThread`
2. remainin
...

💬 stratospher commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-2134500918)
thank you for the helpful reviews @sr-gi!

I've updated the PR:
- `EARLY_KEY_RESPONSE` test doesn't have `magic_sent` now and the `initial_v2_handshake()` function is cleaner
- `EARLY_KEY_RESPONSE` test does sending of 4 bytes ellswift (which match network magic) + remaining ellswift and garbage bytes on `MainThread` - both are done manually now
- Added v2 handshake timeout log in 7d07daa


> What is weird is that the test is expecting a specific log message, which seems to also mat
...

👍 alfonsoromanz approved a pull request: "test: Assumeutxo: snapshots with less work should not be loaded"
(https://github.com/bitcoin/bitcoin/pull/29428#pullrequestreview-2082042285)
Re ACK df6dc2aaaeffc664006b86ee8c8797dc484ec40e. Make is successful and the test passes.

<img width="500" alt="Screenshot 2024-05-28 at 09 55 15" src="https://github.com/bitcoin/bitcoin/assets/19962151/e3e974e8-a507-49c9-8d77-6457f6f95443">
<img width="500" alt="Screenshot 2024-05-28 at 09 58 39" src="https://github.com/bitcoin/bitcoin/assets/19962151/f2e020ed-efc6-4854-97b5-f919ac1e0cdf">

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2134614071)
> if you have more thoughts on it

Whenever I talk to people interested in using Stratum v2 I find it difficult to explain all the moving parts they need to install and configure.

Install:
* a separate Bitcoin Core branch (temporary, pending #29432 merge)
* a job declaration client
* a sv1-sv2 translator (temporary, pending sv2 firmware support)

Configure:
* point job declaration client to Bitcoin Core, to the pools job declaration server and to the pool itself
* point translator to
...

👍 dergoegge approved a pull request: "fuzz: Handle missing BDBRO errors"
(https://github.com/bitcoin/bitcoin/pull/30172#pullrequestreview-2082119176)
tACK 44e071be1d3bc9cd48c776bcd49310d0f8189f5f - No longer running into errors when fuzzing

💬 maflcko commented on issue "Manually Banning Peers Results in Crash":
(https://github.com/bitcoin/bitcoin/issues/29916#issuecomment-2134668885)
Any update on this, trying in gdb, or a similar debugger?

👍 dergoegge approved a pull request: "refactor: Use type-safe time in txorphanage"
(https://github.com/bitcoin/bitcoin/pull/30170#pullrequestreview-2082156040)
Code review ACK fa6d4891c7815025ab1cd58d0906466af31bb648

💬 dergoegge commented on pull request "refactor: Use type-safe time in txorphanage":
(https://github.com/bitcoin/bitcoin/pull/30170#discussion_r1616846129)
Out of scope for this PR but this global is probably the source of instability in the txorphan harness.

💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1616905695)
nit:
```suggestion
/** Number of objects in the container. m_size <= m_capacity. */
```

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2134764402)
Rebased. I dropped `NO_NOBLE` since Noble has been released. Dropped 8b06d6fb797b1de36abb13f3aaf1a7d1607c2685 after #29660.

💬 foolbear commented on issue "Load PSBT error: Unable to decode PSBT":
(https://github.com/bitcoin/bitcoin/issues/30070#issuecomment-2134773965)
In the above issue, PSBT was created from GUI - send - create unsigned. If I create it using 'walletcreatefundedpsbt,' there is no problem. in main net.

👍 hebasto approved a pull request: "[27.x] Backports and rc1"
(https://github.com/bitcoin/bitcoin/pull/30092#pullrequestreview-2082279671)
re-ACK f30e23e4b35b52728ebaa1a63ec9a6feac51ebb4,

💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1616940538)
What is the purpose of this line here? It doesn't look like a pre-condition for the function arguments. It seems it should be used as a post-condition for `m_capacity` mutators (including this function) and `m_offset` mutators, no?

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2134851003)
I had to increase the fetch depth in 6398efbe145fb297957060600615262d59f2b47c, because otherwise the test-each-commit task can fail with: `fatal: bad revision '^^@'` and `You are not currently on a branch`, see [example](https://github.com/Sjors/bitcoin/actions/runs/9267073776/job/25492701755?pr=30#step:4:42).

This happened in https://github.com/Sjors/bitcoin/pull/30 because it doesn't build on a merge commit but rather on top of the branch in this PR. I'm surprised it doesn't also fail on a
...

👍 willcl-ark approved a pull request: "[27.x] Backports and rc1"
(https://github.com/bitcoin/bitcoin/pull/30092#pullrequestreview-2082352837)
reACK f30e23e4b35b52728ebaa1a63ec9a6feac51ebb4

Didn't review the newly-added individual translations, but apart from that changes LGTM.

💬 Sjors commented on pull request "ci: Reintroduce fixed "test-each-commit" job":
(https://github.com/bitcoin/bitcoin/pull/28497#discussion_r1616978286)
I'm surprised this doesn't cause a failure on pull requests that build on other pull requests.

If pull request B has 3 commits that build on top of pull request A with 10 commits, then `FETCH_DEPTH` would be 5. I believe that should cause `git log --merges -1` below to fail, because it won't have the merge commit.

But looking at #28201 this seems to work fine. But when pull request A is on this repo and pull request B is on a fork repo it doesn't, it seems: https://github.com/bitcoin/bitco
...

💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1616978938)
Does it make sense to skip this call if `std::is_trivially_destructible_v<T>`?

💬 Sjors commented on pull request "ci: Reintroduce fixed "test-each-commit" job":
(https://github.com/bitcoin/bitcoin/pull/28497#discussion_r1616980946)
@ryanofsky fixed a simpler case of this in #28573, where PR B has 1 commit. Though I'm confused why he ran into it while #28201 doesn't.

💬 vasild commented on pull request "net: make the list of known message types a compile time constant":
(https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1616990466)
This was tried: https://github.com/bitcoin/bitcoin/pull/29421#discussion_r1491140321

Here is a draft that changes to `std::string_view` the following:

* `NetMsgType::*`
* `ALL_NET_MESSAGE_TYPES`
* `CMessageHeader:pchCommand` (renamed to `CMessageHeader:m_command`)
* `CNetMessage::m_type`
* `V2_MESSAGE_IDS`
* `V2MessageMap::m_map`
* `V2Transport::m_send_type`
and the functions that handle message types.

The scope of this can be reduced by converting the `std::string_view` to `std:
...