💬 Sjors commented on issue "--with-sanitizers=float-divide-by-zero crash with -debug=bench in Chainstate::ConnectTip":
(https://github.com/bitcoin/bitcoin/issues/27635#issuecomment-1549472481)
Opened #27673 which drops the offending metric.

💬 sipa commented on pull request "Enable HW-accelerated implementations of SHA256 for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/24773#discussion_r1195017841)
Yeah, MSVC does not support inline asm at all. You have to use intrinsics.

💬 Sjors commented on pull request "ConnectTip: don't log total disk read time in bench":
(https://github.com/bitcoin/bitcoin/pull/27673#discussion_r1195022903)
Fwiw I think this warning is useful because many of the bench logs use `/ num_blocks_total` and `ConnectTip` has an `assert(num_blocks_total > 0)` just a few lines down.

💬 ajtowns commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1195027764)
We don't accept transactions while in IBD, so sending an INV first might be an easy way to catch that condition too. Checking that we're on the same tip, and respecting feefilter might also catch those cases, and be worth doing anyway though.

📝 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.