💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1516922824)
> > the resulting file has roughly 2x the size of the compact-serialized UTXO set (this is mostly due to encoding txids and scriptpubkeys as hex-strings rather than bytes)
>
> What is the rationale for encoding as text rather than bytes? SQLite can store byte values as BLOBs.
Fair question. There was already some discussion in #24952 about whether to store txids/scriptPubKeys as TEXT or BLOB, see https://github.com/bitcoin/bitcoin/pull/24952#pullrequestreview-952102140, https://github.com/
...
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1516922824)
> > the resulting file has roughly 2x the size of the compact-serialized UTXO set (this is mostly due to encoding txids and scriptpubkeys as hex-strings rather than bytes)
>
> What is the rationale for encoding as text rather than bytes? SQLite can store byte values as BLOBs.
Fair question. There was already some discussion in #24952 about whether to store txids/scriptPubKeys as TEXT or BLOB, see https://github.com/bitcoin/bitcoin/pull/24952#pullrequestreview-952102140, https://github.com/
...
💬 fanquake commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1516926753)
Ubuntu 23.04 has been released: https://lists.ubuntu.com/archives/ubuntu-announce/2023-April/000289.html. So the Google Cloud images should be available soon.
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1516926753)
Ubuntu 23.04 has been released: https://lists.ubuntu.com/archives/ubuntu-announce/2023-April/000289.html. So the Google Cloud images should be available soon.
👋 fanquake's pull request is ready for review: "ci: use LLVM/clang-16 in native_asan job"
(https://github.com/bitcoin/bitcoin/pull/27360)
(https://github.com/bitcoin/bitcoin/pull/27360)
💬 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),
...
(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.
(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`.
(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
...
(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
...
(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
(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)
(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
...
(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
...
(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.
(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
(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...
(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!
(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?
(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
...
(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
(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?
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1517487946)
Is this rfm with two reviews?