💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1258057941)
3 of the PRs are mine (I don't care about rebasing) and the package relay stuff is gonna conflict either way, so I'd prefer keeping it as is.

📝 MarcoFalke opened a pull request: " streams: Add XorFile "
(https://github.com/bitcoin/bitcoin/pull/28060)
This adds a new type, which has the `AutoFile` streams interface, but can roll an XOR pattern while reading or writing to the underlying file.

This is needed for https://github.com/bitcoin/bitcoin/pull/28052, but can also be used in any other place, where `AutoFile` can be used.

Also, there are tests, so I've split this up from the larger pull to make review easier, hopefully.

💬 MarcoFalke commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1628813020)
Also, I don't think this is a fix for https://github.com/bitcoin/bitcoin/pull/27444/files#r1165491936. This will still fail outside the CI. Here, you are only modifying the CI to use a different version of valgrind.

💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1628906716)
Hey there, I fixed the descriptions as suggested, and ran the tests and was able to build Bitcoin Core locally successfully.

💬 MarcoFalke commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1258239050)
Stray file?

💬 furszy commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#issuecomment-1628936305)
> Also it may be good to name the commit that introduced the bug. Otherwise it will be harder for reviewers to see if there was a bug in that commit or if the change was intentional. Also, reviewers will be missing context.

Ok. It was introduced within the test 87a1108c8.
Still, this isn't as serious as you imagine. It is just an extra print. 100% sure that wasn't intended.

The test is exercising a specific fatal error (checking that is being triggered), and doing so it makes the node cal
...

💬 MarcoFalke commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#issuecomment-1628940734)
Are you sure, because locally I don't see it in 25.0:

```
$ ./bitcoin-25.0/bin/test_bitcoin -t validation_chainstatemanager_tests/chainstatemanager_snapshot_completion_hash_mismatch
Running 1 test case...

*** No errors detected

💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1258250019)
Oh I'm sorry I must have forgotten to discard everything other than `mining.cpp`. Should I revert this commit, or something else?

🤔 MarcoFalke reviewed a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1521991201)
Looks like the tests fail

💬 MarcoFalke commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258201606)
225e213110602b4fd1d345167f5f92d26557f6c1: Why ignore the return value? This makes the tests fail

💬 MarcoFalke commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258195719)
18bf3abc2babbe066e5d4b5798209826316c8295: Why is this atomic, when only a single thread has access? Or is the other code multi thread safe?

💬 furszy commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#issuecomment-1628956370)
> Are you sure, because locally I don't see it in 25.0:
>
> ```
> $ ./bitcoin-25.0/bin/test_bitcoin -t validation_chainstatemanager_tests/chainstatemanager_snapshot_completion_hash_mismatch
> Running 1 test case...
>
> *** No errors detected
> ```

Ok good, found the commit. 6eb33bd0c21b3e075fbab596351cacafdc947472 introduced it. It removed the `mock_shutdown` function from the test.

💬 furszy commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#issuecomment-1628960452)
cc @TheCharlatan then. Thanks @MarcoFalke.

💬 MarcoFalke commented on pull request "test: make assumeUTXO test capture the expected fatal error":
(https://github.com/bitcoin/bitcoin/pull/28050#issuecomment-1628961804)
lgtm ACK 3e8bf2e10c26ef9c2f58307b523e4b674ac97a2c

💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1629006969)
@ryanofsky separate tests PR is ready for review: https://github.com/bitcoin/bitcoin/pull/27850

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258287264)
At the end of the PR, two threads end up accessing the variable. The `BaseIndex::Init` call stays in the main thread, while the `BaseIndex ::StartBackgroundSync` call is moved to the 'loadblk' thread (now called 'initload')

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258307033)
upps, fixed the intermediate commit. Thanks.

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258307306)
done

🤔 furszy reviewed a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1522158509)
Updated per feedback, thanks Marco and TheCharlatan.

Changes:
* Fixed bug in one of the intermediate commits (per [comment](https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1258201606)).
* Inlined an if/else in one of the intermediate commits (per [comment](https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1257458241)).

💬 MarcoFalke commented on pull request "test: Check expected_stderr after stop":
(https://github.com/bitcoin/bitcoin/pull/28028#discussion_r1258317110)
thx, done