🤔 glozow reviewed a pull request: "test: MiniWallet: respect passed feerate for padded txs (using `target_weight`)"
(https://github.com/bitcoin/bitcoin/pull/30162#pullrequestreview-2077346481)
Concept ACK!
(https://github.com/bitcoin/bitcoin/pull/30162#pullrequestreview-2077346481)
Concept ACK!
💬 sipa commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613746327)
Hmm, this is a good point. The `RingBuffer` class' interface isn't a ring buffer, but a deque; the implementation happens to use a ring buffer. Suggestions/bikeshedding for a better name welcome.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613746327)
Hmm, this is a good point. The `RingBuffer` class' interface isn't a ring buffer, but a deque; the implementation happens to use a ring buffer. Suggestions/bikeshedding for a better name welcome.
💬 sipa commented on pull request "util: add RingBuffer":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613757739)
"Integral in Rage" sounds like a high-school metal band.
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1613757739)
"Integral in Rage" sounds like a high-school metal band.
💬 fjahr commented on pull request "doc, rpc: Release notes and follow-ups for #29612":
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1613761372)
My thinking here was that if we don't know our own network then something must be really wrong and it's probably a developer error. But I guess an exception is fine, so I removed the assert.
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1613761372)
My thinking here was that if we don't know our own network then something must be really wrong and it's probably a developer error. But I guess an exception is fine, so I removed the assert.
💬 fjahr commented on pull request "doc, rpc: Release notes and follow-ups for #29612":
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1613761550)
done
(https://github.com/bitcoin/bitcoin/pull/30167#discussion_r1613761550)
done
💬 sr-gi commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2129982443)
Some more benchmarks.
MacBook M3 Max:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,678.30 | 595,841.56 | 0.4% | 1.11 | `LinearizeNoIters16TxWorstCase`
| 4,975.16 | 200,998.49 | 0.1% | 1.10 | `LinearizeNoIters32TxWorstCase`
| 10,624.85 | 94,118.94 | 0.1% | 1.10 | `LinearizeNoIters48TxWorstC
...
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2129982443)
Some more benchmarks.
MacBook M3 Max:
| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 1,678.30 | 595,841.56 | 0.4% | 1.11 | `LinearizeNoIters16TxWorstCase`
| 4,975.16 | 200,998.49 | 0.1% | 1.10 | `LinearizeNoIters32TxWorstCase`
| 10,624.85 | 94,118.94 | 0.1% | 1.10 | `LinearizeNoIters48TxWorstC
...
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1613772440)
Yes! That's why the next commit removes it. The reason for the split up into 2 commits is that I wanted to make the previous commit as "move-only" as possible so it is easier to review (especially with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`).
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1613772440)
Yes! That's why the next commit removes it. The reason for the split up into 2 commits is that I wanted to make the previous commit as "move-only" as possible so it is easier to review (especially with `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`).
💬 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!)