💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1613775486)
yes, the proper way would be to set a deadline with a monotonic clock, instead of using the same timeout every time
though, everything considered, the gateway can only hold up this thread; it's not like the rest of bitcoind is waiting on it, and if the gateway is not to be trusted then it's not like we're going to get any useful mappings anyway
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1613775486)
yes, the proper way would be to set a deadline with a monotonic clock, instead of using the same timeout every time
though, everything considered, the gateway can only hold up this thread; it's not like the rest of bitcoind is waiting on it, and if the gateway is not to be trusted then it's not like we're going to get any useful mappings anyway
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1613776595)
True - on one hand this should make it minimally faster - on the other hand having simpler function signatures is also nice. I don't think it really matters much either way, parsing an arg once more can't be very expensive and this is not on any critical path anyway, so I kinda like the current way more.
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1613776595)
True - on one hand this should make it minimally faster - on the other hand having simpler function signatures is also nice. I don't think it really matters much either way, parsing an arg once more can't be very expensive and this is not on any critical path anyway, so I kinda like the current way more.
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1613776822)
see previous comment
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1613776822)
see previous comment
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1613777848)
Good point! The 30s timer in `QueryDNSSeeds` was introduced recently, I hadn't really thought about this during the last rebase.
I'll add this to the cleanup commit after the move.
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1613777848)
Good point! The 30s timer in `QueryDNSSeeds` was introduced recently, I hadn't really thought about this during the last rebase.
I'll add this to the cleanup commit after the move.
📝 BenWestgate opened a pull request: "doc: Change GiB to GB in release-process.md"
(https://github.com/bitcoin/bitcoin/pull/30171)
This will make it consistent with the Welcome GUI and code comments which use "GB" for the `m_assumed_blockchain_size` and `m_assumed_chain_state_size` values.
Since they are used as GB values to calculate disk space in GB needed in the GUI, measuring size in GiB here will cause users to run out of space without a graphical warning. The error between units is over 45 GB for the full chain.
This doc is the only place we say they are GiB, and while an 8-10% overhead can account for the unit
...
(https://github.com/bitcoin/bitcoin/pull/30171)
This will make it consistent with the Welcome GUI and code comments which use "GB" for the `m_assumed_blockchain_size` and `m_assumed_chain_state_size` values.
Since they are used as GB values to calculate disk space in GB needed in the GUI, measuring size in GiB here will cause users to run out of space without a graphical warning. The error between units is over 45 GB for the full chain.
This doc is the only place we say they are GiB, and while an 8-10% overhead can account for the unit
...
💬 nsvrn commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2130009520)
Not sure if useful in this context but RocksDB also has some option which separates the values from keys into a separate log file, which is apparently useful in case values are very large in comparison to the keys and an optimization that works better for SSD drives.
https://rocksdb.org/blog/2021/05/26/integrated-blob-db.html
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2130009520)
Not sure if useful in this context but RocksDB also has some option which separates the values from keys into a separate log file, which is apparently useful in case values are very large in comparison to the keys and an optimization that works better for SSD drives.
https://rocksdb.org/blog/2021/05/26/integrated-blob-db.html
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-2130012490)
> I feel like I'm missing something here though. Before the changes of this pull, the fixed seeds would be pulled after a minute of trying DNS seeds and not reaching the desired peer goal. However, after this pull `ProcessFixedSeeds` is called sequentially after `QueryDNSSeeds`. And the latter has no exit logic based on how long that has been running, meaning that if both are set `ProcessFixedSeeds` will never trigger (?).
>
> PS: Well, technically that's not completely accurate. This will fi
...
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-2130012490)
> I feel like I'm missing something here though. Before the changes of this pull, the fixed seeds would be pulled after a minute of trying DNS seeds and not reaching the desired peer goal. However, after this pull `ProcessFixedSeeds` is called sequentially after `QueryDNSSeeds`. And the latter has no exit logic based on how long that has been running, meaning that if both are set `ProcessFixedSeeds` will never trigger (?).
>
> PS: Well, technically that's not completely accurate. This will fi
...
💬 sipa commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613785731)
Done.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613785731)
Done.
💬 sipa commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613785818)
Done.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613785818)
Done.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1613785831)
> The spec recommend trying again with ever shorter intervals, but no less than 4 seconds, as the deadline approached. But it seems fine to not bother.
Right, the retrying is not according to spec-because i had the same thought. It seems overkill to adaptive timing for something that's meant to interact with the local router. If it doesn't have enough capacity to handle a PCP/NAT-PMP packet it sure won't be routing a lot of traffic. i was more afraid that bitcoind (or the system it's running
...
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1613785831)
> The spec recommend trying again with ever shorter intervals, but no less than 4 seconds, as the deadline approached. But it seems fine to not bother.
Right, the retrying is not according to spec-because i had the same thought. It seems overkill to adaptive timing for something that's meant to interact with the local router. If it doesn't have enough capacity to handle a PCP/NAT-PMP packet it sure won't be routing a lot of traffic. i was more afraid that bitcoind (or the system it's running
...
💬 sipa commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613785897)
Done.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613785897)
Done.
💬 sipa commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613785977)
Done.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613785977)
Done.
💬 theStack commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613786034)
In Rust they call it [`VecDeque`](https://doc.rust-lang.org/std/collections/struct.VecDeque.html), maybe that's a naming option? (It even rhymes!)
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613786034)
In Rust they call it [`VecDeque`](https://doc.rust-lang.org/std/collections/struct.VecDeque.html), maybe that's a naming option? (It even rhymes!)
💬 sipa commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613786106)
Done.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613786106)
Done.
💬 sipa commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613786217)
Done (and more).
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613786217)
Done (and more).
💬 BenWestgate commented on issue "Welcome screen uses GiB values as GB, risking running out of space":
(https://github.com/bitcoin-core/gui/issues/821#issuecomment-2130014340)
On further analysis, the code comments in chainparams.h also think these values should be GB so I understand the origin of the gui error. It seems the release-process.md is the odd one out of the 3. This issue can be closed if the doc: PR below is ACK'd.
https://github.com/bitcoin/bitcoin/pull/30171
(https://github.com/bitcoin-core/gui/issues/821#issuecomment-2130014340)
On further analysis, the code comments in chainparams.h also think these values should be GB so I understand the origin of the gui error. It seems the release-process.md is the odd one out of the 3. This issue can be closed if the doc: PR below is ACK'd.
https://github.com/bitcoin/bitcoin/pull/30171
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-2130020267)
[6430bea ](https://github.com/bitcoin/bitcoin/commit/6430bea749d2c5832faeae12e69fe078680e01d3)to [c97bd16](https://github.com/bitcoin/bitcoin/commit/c97bd161212d56c167d3552da3b13db5ab2bc75e):
Addressed feedback by @sr-gi, thanks!
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-2130020267)
[6430bea ](https://github.com/bitcoin/bitcoin/commit/6430bea749d2c5832faeae12e69fe078680e01d3)to [c97bd16](https://github.com/bitcoin/bitcoin/commit/c97bd161212d56c167d3552da3b13db5ab2bc75e):
Addressed feedback by @sr-gi, thanks!
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2130021361)
1cbbcba42cf1c050022a73cb02b9d385fa184f2e: squashed all fixups to clean up the list of commits, no other changes
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2130021361)
1cbbcba42cf1c050022a73cb02b9d385fa184f2e: squashed all fixups to clean up the list of commits, no other changes
🚀 achow101 merged a pull request: "fuzz: Fix wallet_bdb_parser stdlib error matching"
(https://github.com/bitcoin/bitcoin/pull/30169)
(https://github.com/bitcoin/bitcoin/pull/30169)
📝 achow101 opened a pull request: "fuzz: Handle missing BDBRO errors"
(https://github.com/bitcoin/bitcoin/pull/30172)
Adds error messages that were not being handled. Also removes error messages that no longer exist.
Fixes #30166
(https://github.com/bitcoin/bitcoin/pull/30172)
Adds error messages that were not being handled. Also removes error messages that no longer exist.
Fixes #30166
💬 epiccurious commented on pull request "doc: Change GiB to GB in release-process.md":
(https://github.com/bitcoin/bitcoin/pull/30171#issuecomment-2130032118)
Concept ACK c04edce83db41cfce7a0a2f3418cf830c2e8afb5.
This assumes the value should be displayed in GB. Is there a benefit to doing the calculation and displaying in GiB instead?
(https://github.com/bitcoin/bitcoin/pull/30171#issuecomment-2130032118)
Concept ACK c04edce83db41cfce7a0a2f3418cf830c2e8afb5.
This assumes the value should be displayed in GB. Is there a benefit to doing the calculation and displaying in GiB instead?