Coredev mentioned in IRC - This Week in Bitcoin Core #44
This week inside Bitcoin Core the devs come back from vacation somewhere unknown
Hello 👋 folks, I’m kevkevin. I’m an open-source developer and reporter for Insider Edition. Last week, I reviewed several pull requests from the Bitcoin Core repo.
This week in the weekly IRC meeting, CoreDev was mentioned a few times. There must have been some good conversation there. Be sure to read the IRC logs at the end to learn more.
Merged PR’s
Every week, several changes are officially added to Bitcoin Core. This week, 11 changes were merged. Here are some I found interesting this week.
cli: Replace libevent usage with simple http client by fjahr
As part of the effort to remove libevent as a dependency altogether, Fabian Jahr authored this PR to replace the libevent usage with a simple HTTP client.
In his change, he replaces the libevent-based HTTP client with a simple synchronous implementation that uses theSockclass directly.fuzz: target CDBWrapper by andrewtoth
CDBWrapper abstracts the interaction between Bitcoin Core and its database, currently LevelDB but previously BerkeleyDB. There was no dedicated harness targeting CDBWrapper. Andrew Toth authored this fuzz-testing PR to add coverage for this wrapper.
Also, Toth noted that OSS-Fuzz has a harness for LevelDB, but it fails, so it appears not to be maintained. Adding a harness for it on Bitcoin Core will solve that issue for us.
There are always changes being updated and reviewed in real-time. Here are some notable PR’s that are still up and looking for reviews.
refactor: Add util::Result failure types and ability to merge result values by ryanofsky
Add `util::Result` support for returning more error information and make use of it in LoadChainstate method as an initial application. Followup PRs #25722 and #29700 use it more broadly to return errors and warnings from wallet and kernel functions as well..
IRC meeting notes
Every week on Thursday, there is an IRC meeting. Here are some short notes from that meeting.
stickies-v: #topic Fuzzing WG Update (dergoegge)
dergoegge: no updatestickies-v: #topic Kernel WG Update (sedited)
sedited: released a new rust-bitcoinkernel version with hand-rolled bindings instead of the previous auto-generated ones (reducing our build-time dependencies).
sedited: Other than that, maybe stickies-v has something?
stickies-v: he doesn’tstickies-v: #topic Benchmarking WG Update (l0rinc, andrewtoth)
_andrewtoth_: A bug with leveldb has been reported https://github.com/bitcoin/bitcoin/issues/35298.
_andrewtoth_: A potential fix is in https://github.com/bitcoin-core/leveldb-subtree/pull/61.
_andrewtoth_: l0rinc also discovered the fix improves reindex-chainstate speed on HDDs by 17%.
_andrewtoth_: Downside is around 11% more disk usage measured from the peak of disk usage. Right now an IBD will be 12 GB, this PR increases to 15 GB. But for already synced nodes the increase in disk usage will be very gradual.
_andrewtoth_: Disk usage can be brought back down with a manual -forcecompactdb after sync, but I think we should just accept a little more disk usage on chainstate for not rewriting the entire db every hour.
_andrewtoth_: Any thoughts on this?
fanquake: Can you tldr the bug? My understanding is that we are thrashing peoples disks by continually re-writing (100s GB) chainstate?
Murch[m]: This significantly increases the minimum disk space necessary for a pruned node :-/_
Murch[m]: s/:-/_/:-/\/
stickies-v: also wdym 12GB IDB disk usage? that seems... low?
_andrewtoth_: Seek compaction is a mechanism that will compact files in leveldb due to lots of reads
Murch[m]: stickies-v: Just for the UTXO set
stickies-v: oh, i see. thx
_andrewtoth_: since we do few writes, but lots of reads, it causes all written files to be compacted down to the lowest level
_andrewtoth_: this was happening daily before v30, but v30 writes a small file every ~hour now
_andrewtoth_: it was still a bug doing it every day, but now it’s even worse
Murch[m]: Can the count of reads permitted before compaction be increased?
_andrewtoth_: the purpose of seek compaction was for old spinning disks that took 10ms to seek to a file
_andrewtoth_: Murch: possibly, was suggested by willclark
_andrewtoth_: another option is to make forcecompactdb an RPC, which can be called periodically
Murch[m]: Would it be possible to turn that feature off in general and just call it ~once per week?
_andrewtoth_: but for backports, we can suggest to users to run forcecompactdb after a full sync
_andrewtoth_: I don’t think it’s worth calling it once per week. The amount of new data written every week is < 200MB or so. And that would rewrite the whole db every week.
_andrewtoth_: I think if users notice the increase in disk usage, they can call it then?
Murch[m]: I see, but it would gradually bloat the chainstate by about ~25% over time?
_andrewtoth_: 11% over time, from peak disk usage at that point
_andrewtoth_: peak usage of chainstate historically was, like 13.5 GB? so an IBD will be 11% more than that
_andrewtoth_: if you run today, you will have a compacted db, so it will be 11% more than today
Murch[m]: Ah, I see, that’s where the 15 GB comes from
sedited: Re-writing it once a week seems fine though?
dzxzg: what about enabling compaction for dbcache full flushes and disabling it for the interval flushes?
darosior: Tweaking this parameter in our LevelDB forks would only affect disk usage, not performance at all?
abubakarsadiq: > but for backports, we can suggest to users to run forcecompactdb after a full sync
abubakarsadiq: Why can’t we automate this in the backport patch?
fjahr: How sure are we 11% is really the peak? I assume there were a few measurements so far but it could still be worse on some systems/circumstances we may be missing?
darosior: Could also just do it on startup if that’s easier than setting a timer?
_andrewtoth_: This issue in particular does not want it done at startup https://github.com/bitcoin/bitcoin/issues/29662
_andrewtoth_: re: performance, there will be some more files in the db tree, but we have bloom filters for all of them so skipping is cheap
_andrewtoth_: sedited: I think not rewriting it is the intended operation of leveldb. If users care about 11% less disk space (which we have >100x than chainstate in blocks dir), they can run the option?
_andrewtoth_: dzxzg: that would be dependent on the user’s dbcache setting as well
Murch[m]: Well, not rewriting 10 GB every hour seems like the most pressing aspect of this.
_andrewtoth_: it is my opinion that seek compaction is a bug and should just be removed
darosior: Doing what LevelDB considers normal operations and having a mechanism to optimize disk space (whatever it is) sounds good to me.
_andrewtoth_: abubakarsadiq: i don’t think we should automate it, I think we should just report to the users there may be higher disk space, and run forcecompactdb if desired
darosior: By mechanism giving the option to user, either as a startup option or an RPC command.
darosior: By mechanism i mean*
_andrewtoth_: The startup option is already there, but I think RPC is more ideal
_andrewtoth_: but, for backports the startup option is fine
Murch[m]: If there is already a startup option, an RPC shouldn’t be a too heavy lift, I take it
_andrewtoth_: Murch: yes, I believe l0rinc is already working on that
_andrewtoth_: fjahr: there are measurements in the PR, and a more lengthy explanation
abubakarsadiq: _andrewtoth_: My question was why? Specifically, I assume everyone is going to desire more disk space; is there an implication of doing so that we want users to opt into or not?
Murch[m]: Interesting issue. Your mitigation approach sgtm then
_andrewtoth_: I don’t think it can get worse, but more eyes would be good
Murch[m]: abubakarsadiq: My understanding is that users are worried about the increased writes exhausting their SSDs’ limited write capability
_andrewtoth_: abubakarsadiq: the trade off is more disk IO. It will rewrite 10s of GBs each time. It will really only save 3GB if you did a fresh IBD. Otherwise, you will be rewriting everything for not much savings.
dzxzg: esp. for pruned nodes: i assume that if people are running with prune=650 they might care
Murch[m]: _andrewtoth_: Maybe one automatic compaction at the end of IBD would make sense
stickies-v: yeah one after IBD seems like a sane default?
stickies-v: would prefer not adding an rpc for this if we can avoid it
abubakarsadiq: _andrewtoth_: Murch[m]: makes sense now, thanks.
darosior: I think the startup option should be enough yes
darosior: Doing it after IBD sounds good too
_andrewtoth_: can we track if a user did from genesis easily, and then got to tip?
stickies-v: leveldb is an implementation detail and exposing it through our public stable interface is a bit messy
cfields: +1 on avoiding an rpc. I think the number of people who will be able to use that knowingly and correctly is single-digits.
darosior: AFK
Murch[m]: Lunch? ^^
stickies-v: even startup option i would probably find overkill, unless there is clear demand for it
_andrewtoth_: ok, having the startup option exists undocumented already, but we can find a way to do compaction first time the user gets to tip.
cfields: stickies-v: agreed. Same for any option. If we can’t decide when to do it automatically, we shouldn’t expect a user to be able to know when to do it manually.
abubakarsadiq: +1 cfields: I think if we can make that decision for users, it’s more ideal
Murch[m]: +1 cfields
stickies-v: anything else here _andrewtoth_ ?
_andrewtoth_: Closed #31132 and reopened as #35295. Got some good review there that I need to address (thanks!), more review welcome. That’s it from me, thanks!
stickies-v: thanks for the comprehensive overview, sounds like you got some useful feedbackstickies-v: #topic Silent Payments WG Update (Novo__)
theStack: The libsecp SP module PR https://github.com/bitcoin-core/secp256k1/pull/1765 got some momentum and fresh reviewer eyes after coredev (thanks!). In core, Novo__ opened #35301 and #35302 now (second takes of previously #28122 and #28201, respectively) based on that. Review would be much appreciated.
johnny9dev: Sorry I have to get going. Here is my qml update. https://github.com/orgs/bitcoin-core/projects/1/views/3 is updated. We’re making steady progress and still on track to have the project in a state where we feel it is worthwhile to start getting feedback from a wider group in the middle/end of June.stickies-v: #topic Libevent removal (pinheadmz, fjahr)
pinheadmz: Ironing out edge cases in #35182. Running fuzzers 24/7.
pinheadmz: Antithesis caught something that I thought I fixed, but actually just triggered it again this morning doing a DoS test. It’s an assertion in the destructor which I think we can relax, and just delete connections that are taking to long (>60s) to close. Might needs hodlinator ‘s opinion on that before pushing...
pinheadmz: Janb84 grok-claude caught something which is not hard to fix but is harder to test because of all the nasty global state in httpserver.cpp, so trying to figure out what to include in this PR and what to punt to a follow up. Definitely the first follow-up de-globalizing http will be a very satisfying refactor.
pinheadmz: fjahr ?
fjahr: I just keep addressing review comments on the #34342 as quickly as possible. It feels like it’s getting very close though, thanks to the reviewers :)
stickies-v: alright, looks like that’s it for this week’s WG updates
stickies-v: Anything else to discuss?
jurraca: a few ASmap things
jurraca: looking for one more ACK on https://github.com/bitcoin-core/asmap-data/pull/48
jurraca: opened a PR to merge the prototype asmap.sigs repo into bitcoin-core/asmap-data as discussed at CoreDev https://github.com/bitcoin-core/asmap-data/pull/50
jurraca: and next collab run is set for June 4th, two weeks from today https://github.com/bitcoin-core/asmap-data/issues/51
Murch[m]: The CI workers seem to be very slow for the BIPs repository this morning. I assume that this also applies to the Bitcoin Core PRs. Anyone know what’s going on there?
fanquake: I wanted to bring up #35319
instagibbs: +1 fanquake
fanquake: My understanding is that “normal” -privatebroadcast usage might actually just leak your IP.
fanquake: I’m wondering if we should alert users in some way (mailing list / website?).
fanquake: Seems reasonable if there was some expectations of privacy here
Murch[m]: Also, for any BIP authors in this chat. I started the BIP metadata review that was future work for the BIP3 deployment. If you have any Informational BIPs that should be Specification BIPs, you will probably see a PR in that regard (or can open it yourself!). Similarly any BIPs that have been in Draft for years, will be proposed to be moved to Complete or Closed where applicable, or further input will be sought.
fanquake: Also there doesn’t yet seem to be consensus on how to fix / test
Murch[m]: So, if you want to help, it would be awesome if you look over your own BIPs in that regard.
instagibbs: +1 on some communication at least, brand new opt-in feature so impact is hopefully limited
eugenesiegel: there are a couple different tests, I haven
sedited: instagibbs can you provide a tldr?
Murch[m]: fanquake: What dose “normal” mean in this regard? I thought this happens when a connection downgrades from BIP324 to legacy P2P traffic?
instagibbs: sedited: on v2->v1 downgrade (if peer fails the v2 connection), if we’re using Tor exit node, we leak our ip directly to that target peer
instagibbs: It’s broken unless you connect to honest v2 peers, in other words
instagibbs: or hidden service nodes
_andrewtoth_: or honest v1 nodes
_andrewtoth_: *any v1 nodes
fanquake: (and I guess assume that spies will start trying to exploit given it’s public knowledge)
eugenesiegel: there are a couple different tests, I haven’t taken a look at instagibbs latest test. I’m partial to any functional test that can test without modifying the non-test code. as far as fixes, I think passing around an optional that defaults to nullopt kind of encourages callers to forget to set it? so maybe we could make it explicit? vasild patch does
eugenesiegel: work, just thinking how to make it robust
_andrewtoth_: really only malicious v2 nodes on clearnet
Murch[m]: Doesn’t even require a v2 node, you just have to fudge your service bits
instagibbs: eugenesiegel I made an attempt at robust test-only code changes
instagibbs: but I’m not a net expert, hoping more people look
eugenesiegel: Murch: right, a third party could also do it for some peers
instagibbs: “or honest v1 nodes” no, you leak your ip address to honest v1 nodes, you just hope no one looks at logs :)
instagibbs: oh, if bits arent stuffed, rigth
_andrewtoth_: instagibbs: you leak your ip for v1 nodes too? but it’s using the proxy and not downgrading?
Murch[m]: Wait, it always fails with v1 peers? I thought only on downgrade. That makes it way worse.
instagibbs: I retract my claim, need to re-page in how that part works
cfields: I know it’s waaay out of scope, but I mentioned this a few time at CoreDev... private tx relay would’ve been a great opportunity to launch separate short-lived tor-only instances of CConnman/PeerManager to handle the broadcast. I suspect that we continue to hit edge-cases otherwise :(
cfields: Just something to consider for the future.
eugenesiegel: It only fails on downgrade afaict and with a specific -proxy option. Though if there are v1 peers, a third party could modify service bits and force you to connect v2 and then downgrade
eugenesiegel: So, “-proxy=127.0.0.1:9050” is safe. “-proxy=127.0.0.1:9050=tor” is not safe
eugenesiegel: I have no idea on how widespread the feature is, I think sparrow uses it? I think a mailing list post could be helpful to tell people about the issue
_andrewtoth_: What is the behavior with default proxy options? So unset by the user?
eugenesiegel: _andrewtoth_: I’m not sure, does a user need to set -proxy to enable tor?
_andrewtoth_: no, and default is “disabled” according to bitcoind -h
_andrewtoth_: default also allows making pb connections to clearnet via exit nodes
eugenesiegel: how does bitcoind know the control port then? is it set to the default?
instagibbs: 4 minutes left (this conversation needs to continue anyways)
stickies-v: i’m going to call end of meeting here since we’re coming up on time, feel free to continue ofc. thanks for contributing, everyoneRead here for the full meeting
Releases
No releases this week
Thank you for reading. Be sure to tune in again next week for your updates on Bitcoin Core!
If there are any comments, suggestions, or errors, do not hesitate to reach out or comment