📝 hebasto opened a pull request: "ci: Fix "Number of CPUs" output"
(https://github.com/bitcoin/bitcoin/pull/27674)
This PR is a follow-up to https://github.com/bitcoin/bitcoin/pull/27616:

- on [master](https://api.cirrus-ci.com/v1/task/5809898840129536/logs/ci.log):
```
Number of CPUs \(nproc\): $(nproc)
```

- this [PR](https://api.cirrus-ci.com/v1/task/6495994095861760/logs/ci.log):
```
Number of CPUs (nproc): 32
```

💬 fanquake commented on pull request "ci: A few fixes of `ccache` issues":
(https://github.com/bitcoin/bitcoin/pull/27084#issuecomment-1549536659)
What is the status of this

💬 fanquake commented on pull request "bench: Benchmark all `SHA256` implementations that are available on the system":
(https://github.com/bitcoin/bitcoin/pull/27598#discussion_r1195062719)
> Hm, I don't really like that all of that SHA256 specific code is becoming part of the benchmark framework.

Yea. Concept NACK on the current approach.

💬 TheCharlatan commented on issue "builds: Review use of `@`-prefixed lines in our Makefiles":
(https://github.com/bitcoin/bitcoin/issues/18891#issuecomment-1549542504)
With #27041 merged, I checked the remaining few instances and think they all serve a purpose, so I think this could be closed now.

✅ fanquake closed an issue: "builds: Review use of `@`-prefixed lines in our Makefiles"
(https://github.com/bitcoin/bitcoin/issues/18891)

💬 fanquake commented on issue ""Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives":
(https://github.com/bitcoin/bitcoin/issues/27659#issuecomment-1549545668)
> Based on that, maybe close this issue?

Ok. Seems like this can be followed up with in the [GUI repo](https://github.com/bitcoin-core/gui), in any case.

✅ fanquake closed an issue: ""Create Unsigned" should not show the message: "The amount exceeds you balance" without suggesting alternatives"
(https://github.com/bitcoin/bitcoin/issues/27659)

💬 MarcoFalke commented on pull request "ci: Fix "Number of CPUs" output":
(https://github.com/bitcoin/bitcoin/pull/27674#issuecomment-1549552123)
lgtm ACK 5d49d987319f262ecbef6ff688fc674ed3b5fa43

🤔 jarolrod reviewed a pull request: "guix: document when certain patches can be dropped"
(https://github.com/bitcoin/bitcoin/pull/27668#pullrequestreview-1428445500)
ACK a09269a146b1e32a0e7979692f4455cc2f6faeae

Can confirm the note on the long jump patch ;)

💬 MarcoFalke commented on pull request "ConnectTip: don't log total disk read time in bench":
(https://github.com/bitcoin/bitcoin/pull/27673#discussion_r1195083821)
I'd say it would be fine to move the `++num_blocks_total;` to this line instead, and the `assert` as well. Self-documenting code is better than potentially outdated docs.

💬 fanquake commented on pull request "build: Drop support for g++-8":
(https://github.com/bitcoin/bitcoin/pull/27662#issuecomment-1549581673)
Could adjust the comment here: https://github.com/bitcoin/bitcoin/blob/edd2a8644531e6fb8f64ad6acce86f8fdec9ab26/src/txrequest.cpp#L72 to remove mention of GCC 8.x.

Could pull in this commit, to remove the GCC 8 `-lstdc++fs` check: https://github.com/fanquake/bitcoin/commit/b2632e7c7b7dfc5487629d80e11a8cdeaa649ed9.

🚀 fanquake merged a pull request: "ci: Fix "Number of CPUs" output"
(https://github.com/bitcoin/bitcoin/pull/27674)

💬 Sjors commented on pull request "ConnectTip: don't log total disk read time in bench":
(https://github.com/bitcoin/bitcoin/pull/27673#discussion_r1195105709)
We also call `ConnectTip()` from `TestBlockValidity()` and `VerifyDB()`. Do you know how the counter is supposed to behave in those cases?

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1549605549)
Thank you for the detailed comments on your review @ryanofsky! Very happy to see this being fleshed out into a more general and proper kernel notification interface.

Updated 2f9c2d245360b3fad6d469a76c2916d75b027b57 -> 2c58fbf816d73395167a3046c4ce957829bf45f9 ([chainstateRmKernelUiInterface_3](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_3) -> [chainstateRmKernelUiInterface_4](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_4), [co
...

🤔 fjahr reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1428107827)
Code review ACK ac9adf012925c770dfe523c5b57451f313cc8be5

This is alright, I am just not loving the new `assumed_first` parameter to be honest. But curious to hear what you say @jamesob . If you change it, I promise to re-review it swiftly.

💬 fjahr commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1194913264)
This parameter/api seems kind of unnecessary. Do we actually need/want assumed_first=false anywhere explicitly? Otherwise, I would suggest always returning assumed first and adding a comment that this specific ordering is important.

Alternatively, if it is better to have a default behavior of assumed_first=false then I would still find it simpler to `reverse` the order in the one place where it's needed rather than adding the param.

💬 fjahr commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1195081071)
nit: I would prefer `in_ibd` here

💬 fjahr commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1194862228)
somewhat nit: Why not use `ActiveChainstate()` here? From my understanding, it doesn't functionally make a difference but it makes this a cleaner review since the result of this is replacing `ActiveChainstate()`.

💬 furszy commented on issue "Allow getblockfrompeer to use any peer":
(https://github.com/bitcoin/bitcoin/issues/27652#issuecomment-1549635506)
Ok great sipa and Sjors. Notes taken.

I started implementing this in a separate class `BlockRequestTracker` with its own scheduled task to detect the timeout and re-try + follow the requests progress (which also involves listening to the node disconnection signal etc). But, agree that can also add it to the `FindNextBlocksToDownload` existent mechanism as well.
Could merge both worlds and slowly start decoupling the block downloading logic into a separate module while continue using the `Sen
...

💬 MarcoFalke commented on pull request "ConnectTip: don't log total disk read time in bench":
(https://github.com/bitcoin/bitcoin/pull/27673#discussion_r1195171196)
Maybe all globals can be moved to function scope and the `num_blocks_total` can be duplicated to both functions where it is used?

👍 hebasto approved a pull request: "ci: Remove unused errtrace trap ERR"
(https://github.com/bitcoin/bitcoin/pull/27667#pullrequestreview-1428608268)
ACK fad09b703f5c6d8524a09eef771eb4525f9f3225, tested on Ubuntu 22.04: I can still see warnings from the sanitizers in both unit and functional tests.