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

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

This is an exaggeration, right? Peer doesn't seem completely useless if it doesn't contain the snapshot block because it could contain blocks leading up the snapshot block. It this is right it would be good to clarify, or maybe change to allow requesting blocks from the peer.

💬 MarcoFalke commented on pull request " ci: Fix macOS-cross SDK rsync":
(https://github.com/bitcoin/bitcoin/pull/28273#issuecomment-1679303820)
Changed the fix and added an explanation. Let's hope this works. I presume this was a silent merge conflict between 80d70cb6b04b5a3c13e661c0718a4b5108d55869 and 85e672ab3dfbbeec5255befcbcd239a11536be0e. cc @hebasto

💬 jonatack commented on pull request "p2p: bugfixes, logic and logging improvements":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1294891951)
Good idea, simpler and faster. Done (thanks!)

💬 murchandamus commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#discussion_r1294893286)
I got a crash after 7.4M attempts:

fuzz: ../../src/wallet/test/fuzz/coinselection.cpp:120: void wallet::coinselection_fuzz_target(FuzzBufferType): Assertion `result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed.`

You should be able to replicate the issue with:

$ echo "MP3//////wT/LzMBEABL////////Wv///yWyEABLAADoQP//PP//CAAAPQAAAAAAAAAAPQEAAAAA AAAA/V12+w==" | base64 -d > crash.input
$ FUZZ=coinselection src/test/fuzz/fuzz crash.input

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1294895356)
When one thread waits for another to execute a _particular line of code_, if that line of code is not executed for some time the waiting thread has to assume a timeout failure. I.e. some waiting is unavoidable, I am leaving the code as it is.

Do you consider this resolved?

💬 murchandamus commented on pull request "fuzz: improve `coinselection`":
(https://github.com/bitcoin/bitcoin/pull/27585#issuecomment-1679315436)
The fuzz test crashed for me after 7.4M attempts:

fuzz: ../../src/wallet/test/fuzz/coinselection.cpp:120: void wallet::coinselection_fuzz_target(FuzzBufferType): Assertion `result_bnb->GetChange(coin_params.m_cost_of_change, CAmount{0}) == 0' failed.`

You should be able to replicate the issue with:

```
$ echo "MP3//////wT/LzMBEABL////////Wv///yWyEABLAADoQP//PP//CAAAPQAAAAAAAAAAPQEAAAAA AAAA/V12+w==" | base64 -d > crash.input
$ FUZZ=coinselection src/test/fuzz/fuzz crash.i
...

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1294895367)
Maybe add a comment linking to https://github.com/github/roadmap/issues/528 to say that this should be used, when available?

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1294901116)
Added a comment.

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1679324809)
`55c84c2d3b...ca7a9983eb`: add a comment in the source code, as per https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269953546

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1294906169)
ConsumeIntegral can return 0, so this won't be in the future. Can just drop ConsumeIntegral completely?

💬 ismaelsadeeq commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1294921322)
Thanks, Fix this and the nit in 530ea157ea1c25821711617e5f1da3e95cfdc66d

💬 ismaelsadeeq commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#issuecomment-1679353180)
> Due to the multiple datadirs in the `feature_config_args` test, this assertion actually _does_ fail in `combine_logs.py`:
>
> https://github.com/bitcoin/bitcoin/blob/db7120fbaddff6037734d8e42d1d002d773f8567/test/functional/combine_logs.py#L86
>
> I get the same error on master branch too, so I dunno if it's really up to you and this PR to patch around it or not.

@pinheadmz The update on 530ea157ea1c25821711617e5f1da3e95cfdc66d
is using the `node[0]`'s `bitcoin.conf` to set the option
...