Add functions to construct locators without CChain
ClosedPublic
Actions

Authored by PiRK on Jan 10 2024, 08:45.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABC93ac5d7aa80a: Add functions to construct locators without CChain

Summary

This introduces an insignificant performance penalty, as it means locator
construction needs to use the skiplist-based CBlockIndex::GetAncestor()
function instead of the lookup-based CChain, but avoids the need for
callers to have access to a relevant CChain object.

This is a partial backport of core#25717
https://github.com/bitcoin/bitcoin/pull/25717/commits/ed470940cddbeb40425960d51cefeec4948febe4

Depends on D15124

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Jan 10 2024, 08:45

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 10 2024, 08:45

PiRK requested review of this revision.Jan 10 2024, 08:45

Harbormaster completed remote builds in B26237: Diff 44013.Jan 10 2024, 09:06

PiRK added a child revision: D15129: Utilize anti-DoS headers download strategy.Jan 10 2024, 14:19

@bot build-chronik

teamcity awarded a token.Jan 10 2024, 16:53

Not really requesting changes but these questions need to be answered before this can move

src/chain.h
199 ↗	(On Diff #44013)	Does it need cs_main for Tip() access ?
src/net_processing.cpp
3515 ↗	(On Diff #44013)	Any idea why this was missing in the source material ? Is the lock held at a larger scope ?

This revision now requires changes to proceed.Jan 10 2024, 17:00

PiRK added inline comments.Jan 10 2024, 17:21

src/net_processing.cpp
3515 ↗	(On Diff #44013)	That one looks like a mistake. I probably didn't want to remove it. I'll check why we had it and why core didn't.

PiRK requested review of this revision.Jan 11 2024, 12:20

PiRK added inline comments.

src/chain.h
199 ↗	(On Diff #44013)	I'm not sure. The CChain::Tip function is not annotated to require a lock. On the other hand, the only two callsites for this `CChain::GetLocator` function hold the lock when calling it.
src/net_processing.cpp
3515 ↗	(On Diff #44013)	Actually the source material was taking the lock in a smaller scope, with the `WITH_LOCK` macro The lock was mistakenly not taken by Core in a previous commit, but we didn't do the mistake when we backport it: D14910 Then Core added the lock at a smaller scope in https://github.com/bitcoin/bitcoin/commit/fac04cb6ba1d032587bd02eab2247fd655a548cd if (MaybeSendGetHeaders(pfrom, WITH_LOCK(m_chainman.GetMutex(), return m_chainman.ActiveChain().GetLocator(pindexLast)), peer))