💬 hebasto commented on issue "Mac App UI is freezed most of the time, with no active peers":
(https://github.com/bitcoin/bitcoin/issues/29293#issuecomment-1906194363)
Closing this a the former of reported problems
> App is not responding most of the time, mouse is spinning

is a duplicate of https://github.com/bitcoin-core/gui/issues/299.

And the latter
> there is no stable active peers - there is almost no progress for 2 weeks.

seems better to ask elsewhere, for instance, on https://bitcoin.stackexchange.com/.

✅ hebasto closed an issue: "Mac App UI is freezed most of the time, with no active peers"
(https://github.com/bitcoin/bitcoin/issues/29293)

💬 hebasto commented on issue "GUI unresponsive during IBD":
(https://github.com/bitcoin-core/gui/issues/299#issuecomment-1906195387)
From https://github.com/bitcoin/bitcoin/issues/29293:

> Seems that "connecting to peers" and the beginning of "synchronizing with network" operation block UI thread.

💬 maflcko commented on pull request "ci: Rename tasks (previous releases, macOS cross)":
(https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1906227242)
Yes, Cirrus is merging the code changes with the target branch, but not the config changes with the target branch. Thus, the config will have a reference to a file name, which no longer exists in the code.

Ideally this is fixed upstream (there is an issue already open).

However, in the meantime, I think the error can be ignored.

If you want, and there is no review (or reviewers are happy to re-ACK), you can rebase your pull requests on current master.

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1906252904)
I'm trying to understand this, but confused about why executing "ROLLBACK TRANSACTION" would be expected to fail in sqlite. It seems like the fix in e5217fea1516120643f4a7a55a474648e21e2269 is handling failure to roll back by trying to close and reopen the database. But I don't understand why rolling back a transaction would fail to begin with, unless there was some serious error like disk corruption. Probably to make the this fix clearer and safer it would be good to check the specific error co
...

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1463446504)
done

💬 andrewtoth commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1463449628)
Rebased and added a comment to address this.

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1463455372)
added

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1463455637)
done

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1906326392)
> According to my `range-diff` nothing changed. reACK [18ad1b9](https://github.com/bitcoin/bitcoin/commit/18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d)

for context, this was the reason for the rebase: https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1906227242

💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463486871)
I liked more the previous version which called `fread(3)` here. It was simple stupid. This `>>` is now hard to follow, especially given that it depends on `T`. For `vector` it ends up calling `AutoFile::detail_fread()`. It does not check whether `ferror(3)` occurred.

💬 vasild commented on pull request "Make (Read/Write)BinaryFile work with char vector, use AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29229#discussion_r1463498321)
Same as above, the [dumb](https://en.wikipedia.org/wiki/KISS_principle) version was easier to follow. The new one does not check whether `fclose(3)` failed, but it should. I think that is a serious deficiency in `AutoFile` itself.

`fwrite(3)` may succeed, but if a subsequent `fclose(3)` fails we should consider the data did not make it safely to disk and that the file is corrupted (`fclose(3)` writes any buffered data to disk using `fflush(3)`, so a failure at `fclose(3)` is as bad as failure
...

💬 ryanofsky commented on pull request "ci: Rename tasks (previous releases, macOS cross)":
(https://github.com/bitcoin/bitcoin/pull/29218#issuecomment-1906382136)
Would it maybe make sense to have a wiki page with a reverse chronological list of known CI failures? I think it would be useful to have a simple place to check if there is a known issue when a CI job is failing, but I'm not sure if it would create a maintenance burden.

🤔 vasild reviewed a pull request: "CKey: add Serialize and Unserialize"
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1839192623)
Concept ACK

💬 vasild commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1463523098)
Is it not worth saving `CKey::fCompressed` to disk as well and fully support ser/unser of any `CKey`?

💬 vasild commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1463517690)
Use `secure_allocator` for the vector because it also ensures the data is not swapped out to disk. Then you do not need the `memory_cleanse()` call at the end:

```suggestion
std::vector<unsigned char, secure_allocator<unsigned char>> key_data(32);
s >> MakeWritableByteSpan(key_data);
this->Set(key_data.begin(), key_data.end(), true);
```

💬 vasild commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1463531442)
Wait, I forgot that this is inside a `CKey` method and we have access to the private `keydata` member. Should be possible to write directly to it, without an intermediate vector. Something like:

```cpp
MakeKeyData();
s >> MakeSpanFromCKeykeydataomgwhatisthis(keydata->begin(), keydata->end());
```

🤔 sr-gi reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1829978188)
Approach ACK

I think this looks good overall, I left some potential improvements/questions but feel free to disregard them given this is just for testing

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457677043)
I wonder whether it may be worth defining a `Peer` class for this and move the peer-building logic of `initialize_v2_transport` here. I don't think this would make a huge difference, but it would make accessing the elements of peer cleaner for the callers. e.g. `self.peer.send_L` instead of `self.peer["send_L"]`

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457621863)
Why is 32 here being passed in every tuple if it really is used as a constant?

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1457652467)
Why is `garbage_terminators` being actively deleted here instead of letting the deletion be managed by the interpreter once we go out of context (and `peer` is deleted as a whole)?