💬 TheCharlatan commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#issuecomment-1516940697)
Thank you for the reviews!

Updated bbe05915adad45560907b5a9b58fc55052838c08 -> bbe05915adad45560907b5a9b58fc55052838c08 ([kernelChainType_0](https://github.com/TheCharlatan/bitcoin/tree/kernelChainType_0) -> [kernelChainType_1](https://github.com/TheCharlatan/bitcoin/tree/kernelChainType_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelChainType_0..kernelChainType_1))
* Addressed @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1171175379),
...

💬 TheCharlatan commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1173086418)
The style guide is vague on enums (though I guess you could follow the recommendation for classes)
```
- Braces on new lines for classes, functions, methods.
- Braces on the same line for everything else.
```
and our code (even the most recent additions) has a mix between putting braces on the same line and a new line for enums.

💬 TheCharlatan commented on pull request "refactor: Move chain constants to the util library":
(https://github.com/bitcoin/bitcoin/pull/27491#discussion_r1173086733)
I'm not sure on this one. Granted, `std::string ChainName(ChainType chain)` would make it clear for the caller that this is most likely a label associated with the `ChainType`. However, for consistency I would also have to rename the helper functions to `GetChainName()`, and that seems less clear, because it does not have a typed argument allowing the caller to infer the relationship with `ChainType`.

📝 pinheadmz opened a pull request: "net: use interruptible async getaddrinfo wrapper from libevent for DNS"
(https://github.com/bitcoin/bitcoin/pull/27505)
Closes https://github.com/bitcoin/bitcoin/issues/16778

Bitcoin uses `getaddrinfo` to make DNS requests for DNS seed servers and for adding peers with `-addnode`, `-seednode` and `-connect`. Depending on the platform this can be clunky and a system issue could prevent name resolution from completing at all, blocking the thread and in some cases preventing a clean shutdown.

An attempt was made to switch to the asynchronous `getaddrinfo_a` in https://github.com/bitcoin/bitcoin/pull/4421 but t
...

💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1173095947)
> But my comment isn't really about the accuracy of the timestamp, but rather about the possibility of skipping a block after we have already started scanning. A block's timestamp can be before its parent. The code as written would let us skip scanning a block even if we had scanned its parent. The reason to use MTP is because it can only move forward. The MTP of a block can never be less than the MTP of its parent, it can be equal, but that's fine for us.

Yeah, where I was going is that we r
...

💬 achow101 commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1516954704)
ACK 0076bed45eb2b42111fa3f4c95181393c685a42e

🚀 achow101 merged a pull request: "logging, net: add ASN from peers on logs"
(https://github.com/bitcoin/bitcoin/pull/27412)

💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1516996244)
Rebased and added a refactor to split up `ThreadAddressSeed` into separate functions for DNS and fixed seeds - I will keep in draft a bit longer, until the issue below is resolved.

> While implementing and testing the `ThreadAddressSeed` code organization changes, I ran into a small issue/ behavior change caused by the movement of the fixed seeding into another thread - I need to analyze this closer, marking as Draft for now.

The issue was that if one chooses `-seednode=X`, the idea is to
...

💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1517007742)
Updated per feedback, thanks achow.

Changes:
* Replaced `BLANK_BIRTH_TIME` constant by setting the `m_birth_time` to its max numeric value.
* Tackled a block time variability concern by using the maximum time in the chain (a moving forward only timestamp) instead of the raw block time.
* Extended the grace period to be twice as big as the one used in the rescan process. Even when this is not strictly needed, the rationale is to be a bit more conservative during IBD scanning because it's an
...

📝 amitiuttarwar opened a pull request: "test: prevent intermittent failures"
(https://github.com/bitcoin/bitcoin/pull/27506)
Follow up to #27214 - add an address to the tried table before the new table to make sure a new table collision is not possible.

💬 amitiuttarwar commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#issuecomment-1517186540)
opened #27506 for the test fix

💬 benthecarman commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1517222300)
> I haven't come up with anything that seems like a good reason to vary consensus params though: if you want to test non-bitcoin behaviours, why modify bitcoin to do it?

I just want to open a lightning channel and not need to wait an hour to use it...

💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#issuecomment-1517233030)
Wohoo :tada: Thanks everyone for making this happen!

💬 MarcoFalke commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1517401415)
I can see it on https://console.cloud.google.com/compute/instancesAdd , but CI doesn't for some reason?


💬 MarcoFalke commented on pull request "test: Remove unused sanitizer suppressions":
(https://github.com/bitcoin/bitcoin/pull/27498#issuecomment-1517410478)
CI is red on epoll_ctl: https://cirrus-ci.com/task/5213026740076544?logs=ci#L3469

```
interface_bitcoin_cli.py --descriptors | ✖ Failed | 7 s
ALL | ✖ Failed | 1944 s (accumulated)
Runtime: 210 s
==================
WARNING: ThreadSanitizer: data race (pid=37965)
Write of size 8 at 0x7bb000000300 by thread T26 (mutexes: write M0):
#0 closedir <null> (bitcoind+0xe0e97) (BuildId: fcf645b68d9aefc406c59edc74635a9769b
...

💬 MarcoFalke commented on pull request "Bump python minimum version to 3.8":
(https://github.com/bitcoin/bitcoin/pull/27483#issuecomment-1517460427)
Thanks, fixed typo in commit message

💬 MarcoFalke commented on pull request "move-only: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1517487946)
Is this rfm with two reviews?

💬 MarcoFalke commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1173521423)
Can you check if
```suggestion
image: family/ubuntu-2210
```

exists? Just to make sure the naming scheme is correct?

💬 fanquake commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1173528020)
Pushed

💬 fanquake commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1173528680)
Looks like either the naming is incorrect, or there isn't actually support for non-LTS?