This week in Bitcoin Core - #29
Bitcoin's price is crashing but that doesn't stop the Bitcoin Core devs...
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. Here’s what I found notable.
Merged PR’s
Every week, several changes are officially added to Bitcoin Core. This week, 29 changes were merged. Here are some I thought were interesting from this week.
Split CWallet::Create() into CreateNew and LoadExisting by davidgumberg
This pull request is mostly a refactor because all of this logic already exists. The only difference is that before it was in the CWallet::Create() function. Now there are two distinct functions for CreateNew and LoadExisting.
The win of this PR is that CWallet::Create used a bad heuristic for trying to guess if we should be creating a new wallet or loading in an existing one. That heuristic was that it assumes that wallets with no ScirptPubKeyMans are being created, but in these two issues #32112 and #32111, it is demonstrated that it is not the case.mining, ipc: omit dummy extraNonce from coinbase by Sjors
This change removes the extraNonce from being included by default in the coinbase scriptSig when using the mining IPC interface. This simplifies downstream mining software like Stratum v2 because now the software does not need to include custom logic to strip the scriptSig of that data.
Also, this seems to be only done in the IPC interface, as the existing behavior is preserved for RPCs, test, regtest, and internal mining.fees: fix noisy flushing log by ismaelsadeeq
This is a straightforward PR that updates the flushing log to use debug-level logging for the estimatefee category. This was done because the log can get a bit noisy, and we would rather hide it in the debug estimatefee category.
Logging changes can be boring because they don’t change anything that affects users that much, but it makes debugging for developers easier if done right.
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.
This drops our dependency on
boost::signals2, leavingboost::multi_indexas the only remaining boost dependency for bitcoind.
boost::signals2is a complex beast, but we only use a small portion of it. Namely: it’s a way for multiple subscribers to connect to the same event, and the ability to later disconnect individual subscribers from that event.
btcsignalsadheres to the subset of theboost::signals2API that we currently use, and thus is a drop-in replacement. Rather than implementing a complexslottracking class that we never used anyway (and which was much more useful in the days before std::function existed), callbacks are simply wrapped directly instd::functions.
IRC meeting notes
Every week on Thursday, there is an IRC meeting. Here are some short notes from that meeting.
16:02:43 <fjahr> #topic Net Split WG Update (cfields)
16:03:03 <cfields> I was distracted by the boost::signals2 replacement this week, still no net split update :(
16:03:18 <cfields> I swear I’ll have an update one of these days :)16:03:29 <fjahr> #topic Cluster Mempool WG Update (sdaftuar, sipa)
16:04:02 <sipa> Focus remains #34257, which I think is ready for review.
16:04:27 <sipa> And I think it’s important to have in 31.0, so we don’t need to change mempool ordering in subsequent releases.
16:04:46 <sipa> So is #34023, which is also ready.
16:05:14 <sipa> I’m working on #34138, and I think with those 3, if they don’t get split up further, we can call cluster mempool *DONE*.
16:05:23 <sipa> That’s it for me.
16:05:24 <fjahr> Someone should add #34023 to the milestone :)16:05:43 <fjahr> #topic Benchmarking WG Update (l0rinc, andrewtoth)
16:06:33 <andrewtoth> getting some review on #34165, that’s it from me.
16:07:33 <sipa> andrewtoth: will review more soon16:07:57 <fjahr> #topic v31 feature freeze upcoming in 3 weeks
16:08:04 <fjahr> - Release schedule: #33607
16:08:05 <corebot`> https://github.com/bitcoin/bitcoin/issues/33607 | Release Schedule for 31.0 · Issue #33607 · bitcoin/bitcoin · GitHub
16:08:14 <fjahr> Milestone: https://github.com/bitcoin/bitcoin/milestone/74
16:08:33 <fjahr> Anything to add or remove? Not that I could do that but I hope somebody will ;)
16:09:07 <andrewtoth> #34329 - we are releasing silent payments, but there are no debug logs. Without this it will be difficult to help users troubleshoot. It will also let us collect statistics.
16:09:51 <furszy> silent payments is another monster
16:10:05 <andrewtoth> #33512 fixes lots of unnecessary warnings when running with high dbcache values, since we flush a lot now and dont clear
16:10:14 <andrewtoth> furszy: thanks yes that’s what i meant
16:12:41 <fjahr> Anyone else want to add something to the v31 milestone or has other comments on the release?16:15:13 <fjahr> #topic late proposed meeting topic (marcofleon)
16:15:29 <marcofleon> ah there it is. Sorry in advance for the wall of text
16:15:41 <marcofleon> I’ve been thinking about erlay quite a bit and want to see what others think. I’m leaning toward a NACK, with the information I have. I’m not convinced that the tradeoffs are worth it. As far as I understand:
16:16:03 <marcofleon> Pros: More tx relay (full relay) outbound connections for a less than proportional increase in data sent/received. We could double the full relay outbound connections from 8 to 16 without doubling the bandwidth.
16:16:13 <marcofleon> Cons: Latency across the network increases. In some cases in the simulations, latency almost doubled. Also, the changes to the P2P code that Erlay requires are fairly extensive aka added complexity.
16:16:25 <marcofleon> There is work being done in #28463 on increasing block-relay-only connections. I think the eventual goal there would be to have 8 of these connections. It’s not clear to me that having any more than 8 full relay really provides the network with a big benefit. We already have pretty good tx censorship resistance and so it seems like focusing on the block relay only connections for better eclipse attack protection is
16:16:25 <marcofleon> more beneficial. I’m open to changing this conclusion.
16:16:46 <marcofleon> Basically just looking to gauge the project and try to figure out where we stand on this. Of course we don’t need to have answers right now, but at some point (this year?) would be nice to get a priority ACK/NACK. If we generally agree that we won’t be moving forward with it in the foreseeable future, then it would be good to remove any Erlay related code that’s been merged and focus on increasing block relay
16:16:46 <marcofleon> connections. If we do end up deciding that the tradeoffs are worth it and that developer time should be spent on Erlay, then I would be happy to work on/review it and make it happen. It’s definitely an elegant idea
16:18:20 <sipa> It’s a good discussion to bring up. I agree we should decide to either move forward and pioritize, or cancel entirely.
16:19:54 <instagibbs> would a fresh issue with the discussion be a good place to start it? a recap may be helpful to those (raises hand) who haven’t been paying close attention to the evolution of hte idea over time
16:20:07 <marcofleon> No need for a conclusion now, my point was just to bring it up for now. We can keep it in mind
16:20:28 <lightlike> to be fair, erlay became more relevant with the recent attempts to censor transactions by preventing them from reaching miners (Knots). Although the threshold for that is already quite low (~10%)
16:20:59 <marcofleon> instagibbs: A rehash of conceptual discussion in an issue could be good
16:21:14 <furszy> sr_gi[m] ^^
16:21:20 <fjahr> Agree good to have the discussion, I was also wondering what the recent progress was.
16:21:30 <Murch[m]> I think AJ’s proposal to sync the top of mempools is also relevant in the context
16:22:00 <willcl-ark> sr_gi[m]: has been re-running simulations recently too IIUC, which could be good to have presented in such a recap issue
16:23:17 <stickies-v> lightlike: i think the new private broadcast mechanism might also be useful there to more aggressively broadcast transactions?
16:23:18 <sipa> i think a fresh issue with conceptual discussion, trade-offs, and recent developments would be good
16:23:50 <sipa> stickies-v: that seems largely orthogonal to me, as it’s just about the initial announcement
16:24:12 <marcofleon> nice, that sounds good. Yeah we can get recent updates there too. I know its a big project and simulations and testing are a big part of it, so its important to be thorough
16:24:47 <sipa> the mempool sync idea may matter - with that, we can perhaps tolerate a lower guarantee on reachability after fanout + recon, if it’ll be fixed up later with mempool sync
16:24:49 <stickies-v> but being able to announce it to more peers seems like it should help with reaching more miners?
16:24:56 <marcofleon> My thoughts were based on the simulations I saw last year, maybe something has changed
16:25:05 <stickies-v> (haven’t thought this through, just popped in my head)
16:25:40 <sr_gi[m]> I’ve been rebasing the code recently after private broadcast was merged. I agree with marcofleon that we should have a general decision on whether move forward with it or cancel. I’m happy to do a refresher on all the tradeoffs and pending things if people are interested, and push forward if there is interest in reviewing the code
16:26:41 <marcofleon> sounds good to me
16:27:11 <marcofleon> We can open a fresh issue then. Thanks everyone
16:27:35 <fjahr> Thanks marcofleon! Anything else to discuss?Releases
None 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