Optout21 merges two PRs - This Week in Bitcoin Core #40

This week there was a PR review club after a long time...

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, a user named Optout21 had two PR’s that they authored and merged. Check out the merged PR’s section to read more!

Merged PR’s

Every week, several changes are officially added to Bitcoin Core. This week, 10 changes were merged. Here are some I thought were interesting from this week.

• refactor: Change CChain methods to use references, add tests by optout21
  optout21 refactored the CChain methods, no longer use pointers because of the risk of accidental nullptr dereference. The methods being updated in CChain are Contains(), Next(), and FindFork().
  
  This improves the safety of the CChain methods because there will be a lower likelihood of falling into this nullptr dereference error.
  
  Optout21 concludes that there might be future improvements that make sense.

  Further ideas, not considered in this PR:

  • Change InvalidateBlock() and PreciousBlock() to take references.

  • Change CChain internals to store references instead of pointers

  • Change CChain to always have at least one element (genesis), that way there is always genesis and tip.

  • Check related methods to return reference (guaranteed non-null) -- FindFork, FindEarliestAtLeast, FindForkInGlobalIndex, blockman.AddToBlockIndex, etc.

• test: Clean shutdown in Socks5Server by optout21

  The Socks5Server utility handles multiple incoming connections, which are handled in separate background threads. Its stop() method unblocks and waits for the main background thread cleanly, but it doesn't attempt to wait for the completion of handler threads. There is no guarantee that the handler threads are finished after stop() returns, which can lead to IO errors.

  This pull request by optout21 adds a clean shutdown to Socks5Server to avoid intermittent CI failures.

• Rollback for dumptxoutset without invalidating blocks by fjahr
  Fabian Jahr opened this change because of a need to implement dumptxoutset with a rollback. It was also wanted a way to roll back that did not use invalidateblock and reconsiderblock and instead created a temp copy of the coins DB.
  

  There are a few upsides to this approach.

  • Network activity does not have to be suspended

  • Forks can not interfere with the rollback

But also a few downsides like requiring more disk space, and performance is slower.

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.

• validation: fetch block inputs on parallel threads by andrewtoth

  Problem

  Currently, when fetching inputs in ConnectBlock, each input is fetched from the cache sequentially. A cache miss requires a round trip to the disk database to fetch the outpoint and insert it into the cache. Since the database is read-only during ConnectBlock, we can fetch all inputs of a block in parallel on multiple threads while connecting.

  Solution

  We add a ThreadPool to CoinsViewOverlay to fetch block inputs in parallel. The block is passed to the CoinsViewOverlay view before entering ConnectBlock, which kicks off the worker threads to begin fetching the inputs. The cache returns fetched coins as they become available via the overridden FetchCoinFromBase method. If not available yet, the main thread also fetches coins as it waits.

---

IRC meeting notes

Every week on Thursday, there is an IRC meeting. Here are some short notes from that meeting.

abubakarsadiq: #topic Net Split WG Update (cfields)
abubakarsadiq: I guess no update from cfields, if there is we can come back to the topic
cfields: No update this week, been working on my multi_index replacement.

abubakarsadiq: #topic Benchmarking WG Update (l0rinc, andrewtoth)
l0rinc: #35025 was merged, the deserialization benchmarks are more realistic now.
l0rinc: The untimed setup of nanobench needed a follow-up to make it more intuitive to use, see #35124.
l0rinc: #34641 was split into tiny, focused commits based on the feedback.
l0rinc: Pushed #35128 to speed up `gettxoutsetinfo` - a follow-up is already brewing based on the feedback.
l0rinc: That’s it from me, thanks for the reviews.

abubakarsadiq: #topic QML GUI WG Update (johnny9dev)
johnny9dev: Opened up a PR for new settings pages (bitcoin-core/gui-qml#551) This includes Wallet settings that has details, create backup, password add/update, and wallet deletion. Sign message will likely end up here as well. The PR also includes a Mempool settings page that shows transaction amount and memory usage as well as an input field for updating mempool memory amount. I am currently working on a flow for importing PSBT.
johnny9dev: We have a new contributor as well, pseudoramdom, who will be starting by implementing our RBF design as his first feature.
johnny9dev: With pseudoramdom on RBF, epicleafies working on Receiving and Sign message, and me doing PSBT import I think that just leaves the “Address Book” page as the last feature-parity issue needing a first implementation
johnny9dev: that is all

abubakarsadiq: #topic Libevent removal (pinheadmz, fjahr)
fjahr: I keep getting good review comments on #34342, trying to address them as fast as possible but didn’t get to the comments from yesterday yet :) Waiting for the rebase/re-open to start review on the http server again. That’s it from me.
pinheadmz: Nothing new from me this week
abubakarsadiq: I skipped some working groups because the leads aren’t here. If you have an update please propose the topic
abubakarsadiq: Anything else to discuss?

---

Releases

• 31.0 Release

---

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