💬 MarcoFalke commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-1678954526)
I guess this relates to the TODO as well: https://github.com/bitcoin/bitcoin/blob/80d70cb6b04b5a3c13e661c0718a4b5108d55869/src/blockencodings.cpp#L24

💬 furszy commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#discussion_r1294613398)
The idea was to separate the stderr output from the exception being thrown here. Mainly to get a more readable print when a crash happens. The `FailedToStartError` exception message appears in-between two long stack traces.

But, either way is ok for me.

💬 glozow commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1294614675)
> Requesting the already confirmed transaction is surprising to me.

Yep, was strange to me too, but `AlreadyHaveTx` is peerman's best metric for determining what the missing parents are (validation doesn't say which one(s) are missing). Also afaiu it's considered uncommon to (1) have an orphan (2) have a transaction that spends a very old coin, so I'm guessing this is quite rare.

> Is this expected to change in future work

I'm not aware of any plans to change the legacy way of requestin
...

💬 MarcoFalke commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#discussion_r1294617879)
If you want to keep it, it would also be good to show an example output (CI and non-CI) when this is printed in a "normal" test (not create_cache)

💬 Crypt-iQ commented on issue "compact block fingerprinting":
(https://github.com/bitcoin/bitcoin/issues/28272#issuecomment-1678970122)
I think implementing the TODO might add round-trips and maybe leak what we had in our own mempool prior to block acceptance?

💬 furszy commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#discussion_r1294638606)
Why might the output differ between CI and non-CI runs? The test framework redirect stderr to a file, and this file remains untouched regardless of the execution environment.

Also, the `"normal" test (not create_cache)` tense isn't clear for me. Does it refer to running an individual test locally?

💬 fjahr commented on pull request "rpc: Add dumpcoinstats":
(https://github.com/bitcoin/bitcoin/pull/19792#issuecomment-1678988325)
There doesn't seem to be much interest in this feature currently. If someone needs it, feel free to ping me here and I will reorg/rebase for you.

✅ fjahr closed a pull request: "rpc: Add dumpcoinstats"
(https://github.com/bitcoin/bitcoin/pull/19792)

💬 achow101 commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#issuecomment-1678988739)
ACK fa776e61cd64a5ffd9a4be589ab8efeb5421861a

💬 MarcoFalke commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#discussion_r1294654358)
> Why might the output differ between CI and non-CI runs?

I thought that the test_runner.py may be doing something with stderr, but it looks like it will get printed:

```py
print(BOLD[1] + 'stdout:\n' + BOLD[0] + stdout + '\n')
print(BOLD[1] + 'stderr:\n' + BOLD[0] + stderr + '\n')
if combined_logs_len and os.path.isdir(testdir):
# Print the final `combinedlogslen` lines of the combined logs
```



> Also, the `"n
...

🚀 achow101 merged a pull request: "rpc: Add importmempool RPC"
(https://github.com/bitcoin/bitcoin/pull/27460)

💬 furszy commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#discussion_r1294678025)
> > Why might the output differ between CI and non-CI runs?
>
> I thought that the test_runner.py may be doing something with stderr, but it looks like it will get printed:
>
> ```python
> print(BOLD[1] + 'stdout:\n' + BOLD[0] + stdout + '\n')
> print(BOLD[1] + 'stderr:\n' + BOLD[0] + stderr + '\n')
> if combined_logs_len and os.path.isdir(testdir):
> # Print the final `combinedlogslen` lines of the combined logs
> ``
...

💬 MarcoFalke commented on pull request "test: display abrupt shutdown errors in console output":
(https://github.com/bitcoin/bitcoin/pull/28253#discussion_r1294687210)
> In a first glance, that doesn't seems to print the nodes' stderr. That seems to be related to the job stderr (the test run in a subprocess). And those aren't connected.

Currently, you are reading the node's stderr and the printing it to stdout, so it *should* appear as `print(BOLD[1] + 'stdout:\n' + BOLD[0] + stdout + '\n')`.

> Ok. The result is pretty much the same. Just the stack trace is a bit longer.

The difference should be that the output won't be shown in the CI tail if the tes
...

👍 brunoerg approved a pull request: "net: Continuous ASMap health check"
(https://github.com/bitcoin/bitcoin/pull/27581#pullrequestreview-1578712710)
crACK b87045da22adec42cba28d1c9e9b910ab6cf5380

📝 MarcoFalke opened a pull request: "ci: Fix corrupt macOS-cross depends cache"
(https://github.com/bitcoin/bitcoin/pull/28273)
For some reason the macOS-cross depends cache is corrupt and leads to errors on all builds (master and pull requests): https://cirrus-ci.com/task/4858643863044096?logs=ci#L1291

Fix this by creating a new cache via a new fingerprint via the task name. See https://github.com/bitcoin/bitcoin/blob/cd43a8444ba44f86ddbb313a03a2782482beda89/.cirrus.yml#L69

💬 fjahr commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1294802076)
Thanks, I will change this when I have to retouch.

🤔 ryanofsky reviewed a pull request: "assumeutxo (2)"
(https://github.com/bitcoin/bitcoin/pull/27596#pullrequestreview-1578539655)
Started review (will update list below with progress).
- [X] 6be9fd22ee2b9759cdd30a7bca4d07a9346eb57b First draft of background sync logic (1/23)
- [ ] a62ed3267d69ae8f4e9edf68f4a0f5348fdefe2d bugfix: correct is_snapshot_cs in VerifyDB (2/23)
- [ ] 3b9d3d63d9ccb609ec08da34776f7c5f29d21a20 assumeutxo: disallow -reindex when background syncing (3/23)
- [ ] cdb73e44b7f0d4ea186ba657ddf7809434f0a12a chainparams: add blockhash to AssumeutxoData (4/23)
- [ ] 5ea9abfbac3543f2d2cef90a1528abed3562382
...

💬 ryanofsky commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1294582794)
In commit "First draft of background sync logic" (6be9fd22ee2b9759cdd30a7bca4d07a9346eb57b)

re: https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1290225497

> This look suspiciously similar to `FindNextBlocksToDownload`, is there no way to unify the two?

This seems pretty bad. The most complicated part of the function is copied and pasted basically verbatim, and to make things worse, the comments which explain how it works are removed in the duplicated code.

Would suggest 43b
...

💬 ryanofsky commented on pull request "assumeutxo (2)":
(https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1294850766)
In commit "First draft of background sync logic" (6be9fd22ee2b9759cdd30a7bca4d07a9346eb57b)

This code is just copied and pasted from previous code which was added back in #4468, but `std::max` seems like a bug here, and I would expect it to be `std::min` if the point is to read up to 128 successors of pIndexWalk at a time.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1294877491)
Marking this as "resolved" because I reverted to the original interface of `ASSERT_DEBUG_LOG()` and changed the exception from the destructor to `std::abort()`.

Let me know if you still have concerns (aka if I should "unresolve" it). Thanks, it looks better now!