beacon/light: add CommitteeChain

[zsfelfoldi]

This PR implements CommitteeChain which is a key component of the beacon light client. It is a passive data structure that can validate, hold and update a chain of beacon light sync committees and updates, starting from a checkpoint that proves the starting committee through a beacon block hash, header and corresponding state. Once synced to the current sync period, CommitteeChain can also validate signed beacon headers.

[rjl493456442]

If a fixed root is present at the next period then the update can only be present if it proves the same committee root.

Can you explain why this scenaio is posssible that a "future" fixed root is configured.
If the local chain is stale and we restart it with a latest checkpoint, we can simply
reset the whole committee chain and initializes it with the given latest checkpoint?

I would like to simplify the situation a bit, that committeeChain starts from a external
configured, trusted committee and then extend it with signed updates. Also we can
provide a external API called SetChain or so to manually rewind if something bad
happens. Maybe I miss something?

[zsfelfoldi]

"Future" fixed roots typicaliy do not exist on their own, except right after checkpoint init when we have fixed roots at P and P+1, a committee at P and no updates. In servers connected to a trusted local full beacon node, new updates and committees are always fixed so that a fixed root is set and then the update and committee are added. This ensures that the same syncing mechanism can be used everywhere and it can improve existing updates to better ones that prove the same committee root, but cannot reorg the existing chain even if there is an ongoing attack that somehow manages to generate better sync committee updates (note that this is unfortunately not slashable atm). In this scenario the real chain is still retained by honest servers and clients can manually fix by updating their checkpoint.
All of this is elegant from one viewpoint and maybe complicated from another, but this is not the main reason for its existence. The real reason for the possibility of fixing further roots after init is backward syncing; servers have to sync old data from other server (both the committee chain and other chain/state data) after being initialized, otherwise older data would get harder to access over time. Backward syncing committees is only possible if we first fix the roots (either based on a trusted local beacon node or a historical state proof).

[rjl493456442]

I have two main concerns to challenge the design:

1. Why do we still have the trusted server assumption. I can understand servers and clients start with a trusted checkpoint, but all the later chain extension must be verified, no trust assumption is required, isn't it?

If there is some attacks and servers/clients can easily be switched to attack chain, I will say this sync committee is wrong by design.

2. Can you elaborate how to backward sync the committee chain? The initial external set checkpoint specifies the committee for verifying the updates belong to this period, it's unable to verify any pre-period updates.

[zsfelfoldi]

1. "trusted" in this context means a local beacon node that is connected to a Geth instance acting as a light server; so it is your own node which you should trust over anything else. Clients of course do not need trusted servers.
   The attack vector I mentioned (bribing the supermajority of sync committee signers) is hopefully mostly theoretical but still, I feel it's best to reduce the potential chance of success (actually misleading clients) as the honest servers will still propagate the proper updates.
   Btw there is another, maybe more probable scenario where it makes sense to fix roots according to the local beacon node: if the participation score of the best available update is below the default threshold setting. In this case clients can decide to slightly lower their own threshold but servers should always propagate the best available update so that clients have this option. With a fixed root at the next period, the server's CommitteeChain can accept the update even if its score is below the threshold (now that I checked, I did not add this condition yet but I will do so now).

[zsfelfoldi]

2. with the historical_roots and state_roots trees we can prove any old beacon state from the most recent one and the old beacon state can prove the old committee root. Btw servers will not only backward sync committees but also checkpoints from each other, so that if the client knows an older checkpoint hash it can still be served. The beacon state proofs proving execution block roots is also synced from other servers so that old exec blocks can be proven (this is what replaces the old CHTs). Since beacon state is only generated for new blocks and beacon nodes do not retain it for very long, it is a generally necessary mechanism for our use cases to retain certain parts of the old beacon states on the server side and also to be able to sync it from other servers. This is already implemented and will be a part of a future PR.

[rjl493456442]

Any particular reason to have this function?

I think it should only be used to initialize the committeeChain with some external configured root?
Is it possible to add the root after initialization to force the committeeChain to reorg to some branch?

[zsfelfoldi]

It as also required for backward syncing committees. For servers connected to a trusted local full beacon node, the easiest way to use CommitteeChain is to just set a fixed root for each committee fetched from the local node. Client-servers (light clients that also propagate beacon chain proofs to others) can use state proofs to prove old committee roots and set them as fixed roots.

[rjl493456442]

I think it relies on the assumption that the beacon node is trustful, to make backward sync feasible. I guess it's a valid scenario that some entities want to sync the full committee chain.

But I will just go with anther direction, we can set the initial checkpoint with the earliest one and do "forward" sync then. In this case, we can get rid of the complexity of "backward" sync. IIUC, backward sync just fetches the pre-checkpoints one by one and trust them blindly. It's way more elegant to set the initial checkpoint as early as possible and then extend chain with algorithm.

In order to make this sync viable, we need to ensure the updates belong to past period is still available. Do we have any API to fetch updates of specific period?

[zsfelfoldi]

It is a basic property of PoS that proofs get weaker over time. Clients may choose to trust a checkpoint that is a year old but servers should definitely not light sync the entire chain history from the beginning (or a very old checkpoint). On the other hand, we can easily and trustlessly prove anything from the most recent head which is always the safest starting point. As I wrote above, backward syncing old beacon state data is required anyways for execution root proofs.

[rjl493456442]

Do we support backward committee chain sync?

e.g. We have update list [U2, U3, U4] and committee list [C2, C3, C4, C5], can we extend with update_1?

I guess it's impossible to verify the update_1 lacking of committee_1 right?
If so, should we reject the backward extendable update, and only accept (1) the update of next period (2) the update of last period?

[zsfelfoldi]

It is possible to prove an old committee root through a historical state root from the recent head. You can then add it as a fixed root and fetch the belonging committee and update. This is not used by simple clients but servers (or clients which also serve data) will do backward syncing of committees and checkpoints, so that other clients can sync up from an earlier checkpoint.

[rjl493456442]

Sounds cool. So the checkpoint actually acts as a chain aggregator, through which we can obtain the whole historical chain.

[rjl493456442]

It is possible to prove an old committee root through a historical state root from the recent head

Can you point me the corresponding spec for it?

[zsfelfoldi]

https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/beacon-chain.md#beaconstate
See state_roots and historical_roots. Every new beacon state refers to every previous beacon state so we can prove old committees through the old beacon state (and also old execution block roots which is also required).

[MariusVanDerWijden]

Why do you need both the clock and unixNano here?
I think you can get away with either a function that returns the current time or a mclock.Clock that can either be mclock.Simulated or mclock.System.

[zsfelfoldi]

I need the actual system time for checking signature age. For other use cases I like mclock which is guaranteed monotonic and does not change when the system time is changed, so I need both. By not exposing these in the constructor it becomes a lot less ugly though.

[MariusVanDerWijden]

I can't fully judge it yet, since I don't fully understand how its used, but it looks okay to me

[lightclient]

Probably good to just roll this up into NewCommitteeChain?

[zsfelfoldi]

See #27766 (comment)

[lightclient]

I don't know if this should live in the light package. Maybe would be better in internal? Also is there an equivalent structure for storing regular chaindata? I think it is good to avoid creating new single-use generic data structures when possible.

[zsfelfoldi]

Sure, we can think about it as general purpose but on the other hand I see no other use case for it outside package light right now. I'll leave it as it is for now, according to the 2:1 vote results :)

[lightclient]

Another pretty generic object; is there a reason to have this in light vs. common or internal?

[zsfelfoldi]

See above, I'd leave it for now.

[lightclient]

I find "first" and "next" a bit confusing for a range. IMO "start" and "end" would make much more sense.

[holiman]

Ah, so as I undersand it after looking at it for a while, the Range is actually [first, next[, whereas start/end would be instead have to be [start, end]

[zsfelfoldi]

This is so far the best suggestion, accepted :) At one point it was "first" and "afterLast", that was probably the worst one.

[zsfelfoldi]

I just realized you meant [start, end] and not [start, end[. The reason I want the second value to point after the range it because I want to be able to handle zero length ranges without them being a special case most of the time.

[lightclient]

This method feels a bit overzealous. Wouldn't it be better to simply check if the value would increase the range by only 1, preferably at the end? Is there a reason to also check if the period is contained within the Range?

[zsfelfoldi]

This exact check is needed in many places so I'd rather keep it, maybe change its name if there is a better suggestion. canonicalStore operates on items in continuous ranges and does not care any further about how it's used. It is sometimes expanded on both ends. The CommitteeChain uses three canonicalStores so it also needs this check to check whether it can add a certain item.

[lightclient]

This one also seems too powerful. I think it should be clearer which direction the range is being increased towards.

[zsfelfoldi]

See above.

[lightclient]

What is the purpose of storing historical values here? In the light client spec, the store only stores the latest values and is constant size: https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/sync-protocol.md#lightclientstore

--

Unrelated, this name fixedRoots is a bit ambiguous. Maybe committeeRoots would be better? Should the values in the committees store match the fixed roots values?

[zsfelfoldi]

CommitteeChain is also used by servers. Actually I want light clients to also be able to serve light sync data (committee chain and latest signed head) to other clients. I'd like to move away from the strict client-server separation and make it a typical use case that some node follows the beacon chain as a light client but then processes and serves the state, or just updates and serves a slice of the state.
Another reason to have the chain even as a client-only node is the hopefully largely theoretical possibility of low sync committee signer participation where we might temporarily accept below 2/3 signers but this means that reorgs are also possible. I hope this will never come up on mainnet but the capability of recovering from such a scenario could make such an attack unfeasible.

Also we can later add a prune feature to CommitteeChain so if one really wants to be lightweight then always pruning everything before the latest period is still an option.

[zsfelfoldi]

I agree about "fixedRoots", renamed to "fixedCommitteeRoots".

[lightclient]

Shouldn't this be driven by InsertUpdate?

[zsfelfoldi]

Committees can be added in two ways: either together with an update that proves its validity or separately, based on a fixed committee root. This is how the chain is initialized, the bootstrap data proves two committee roots that we can set as fixed, and one full committee that we can add right away via this function. You cannot call InsertUpdate until at least one committee has been added.
It can also be used by servers who want to backfill some committees based on historical beacon state proofs that can also prove old committee roots. This is why I exported AddFixedCommitteeRoot and AddCommittee separately, because root proving can be done in different ways and getting the actual committee might happen at a different time.

[holiman]

discussion: unexport

[zsfelfoldi]

done

[holiman]

I am not sure that using generics here is a good thing.

1. In the constructor to committee chain, you define on-the-fly decoders and encoders, and initiate the canonicalStores. That is a bit clunky, IMO, and not the most elegant/readable way to organize the code.

2. The caches are all lru of size 100. But typically, we would want to use different cache sizes depending on how heavy the items are. In blockchain, we cache more headers than blocks, for example.

I think this could be written a bit dumbed-down as three explicit stores, perhaps with some shared methods, without expanding the code too much. I'm not 100% sure though, maybe I'll have to try it out.

[holiman]

Here's a proof of concept of a no-generics rewrite: https://github.com/zsfelfoldi/go-ethereum/pull/22/files

+289, -185 LOC, so maybe the generics-version wins. I dunno.

[zsfelfoldi]

I feel like this is a typical use case for generics and it is probably better than the duplication in your version. Maybe I could do encode/decode more elegantly and also use a param for the cache size (though it's not really an issue because they all operate on the same range of periods). I'll think about it.

[namiloh]

How about keeping generics but getting rid of custom encoders, only rely on rlp-encoding things?

[holiman]

For

• SerializeSyncCommitte: it would consume 24627 bytes instead of 24624 bytes.
• LightClientUpdate: no change
• FixedCommitteeRoot (common.Hash): it would consume 33 bytes instead of 32.

I think that would be much better.

Alternatively, if you want to be more explicit, you could do

type codec struct {
	rlp.Encoder
	rlp.Decoder
}

...
type canonicalStore[T codec] struct {
	keyPrefix []byte
...

But then you need to add custom marshallers/unmarshallers on the types.

[holiman]

Why does this type, and, for that matter, the constructor NewComitteeChain, need to be public?
Aren't we going to use them as internals in some upcoming BeaconChain type?

[zsfelfoldi]

No, it is not encapsulated in a bigger structure. In the blsync code it is synced by a sync module in package beacon/light/sync where it is also used by another module to validate incoming new heads.
See the latest blsync branch:
https://github.com/zsfelfoldi/go-ethereum/tree/blsync-nofinal4

[holiman]

You could add two methods:

// Split splits the range into two ranges. The 'fromPeriod' will be the first
// element in the second range (if present).
// The original range is unchanged by this operation
func (a *Range) Split(fromPeriod uint64) (Range, Range) {
	if fromPeriod <= a.Start {
		// First range empty, everything in second range,
		return Range{}, Range{a.Start, a.End}
	}
	if fromPeriod > a.End {
		// Second range empty, everything in first range,
		return Range{a.Start, a.End}, Range{}
	}
	x := Range{a.Start, fromPeriod - 1}
	y := Range{fromPeriod, a.End}
	return x, y
}

// Each invokes the supplied function fn once per period in range
func (a *Range) Each(fn func(uint64)) {
	for p := a.Start; p < a.End; p++ {
		fn(p)
	}
}

Then the canonical store would be simplified... (see below)

[holiman]

Suggested change

	if fromPeriod >= cs.periods.End {
	return
	}
	if fromPeriod < cs.periods.Start {
	fromPeriod = cs.periods.Start
	}
	deleted = Range{Start: fromPeriod, End: cs.periods.End}
	for period := fromPeriod; period < cs.periods.End; period++ {
	batch.Delete(cs.databaseKey(period))
	cs.cache.Remove(period)
	}
	if fromPeriod > cs.periods.Start {
	cs.periods.End = fromPeriod
	} else {
	cs.periods = Range{}
	}
	return
	}
	keepRange, deleteRange := cs.periods.Split(fromPeriod)
	deleteRange.Each(func(period uint64) {
	batch.Delete(cs.databaseKey(period))
	cs.cache.Remove(period)
	})
	cs.periods = keepRange
	return deleteRange
	}

[zsfelfoldi]

Thanks, indeed it's nicer :)

[holiman]

It's a bit funky that you have a db here. Whereas in some cases, like add, you take a backend ethdb.KeyValueWriter.
The cs.db is only used in get and in this iterator here. Feels a bit inconsistent, api-wise.

[zsfelfoldi]

Indeed, it was a bit weird because of the batch operations. Now I always use an external db param.

[holiman]

Is it intentional not to add the resolved value to the cache?

[zsfelfoldi]

It wasn't, fixed.

[holiman]

The logic here is a bit non-idiomatic, and it's kind of difficult to see that this chunk of code ignores one error (namely, if the backend.Get returns an error, it gets silently overlooked).

Please move things around so that you terminate on the happy path

1. Do operation
2. if error, handle
3. do another operation
4. if error handle
   ...
   N. Success all the way

[zsfelfoldi]

Done; I also removed the named return values, I think it's much more readable now.

[holiman]

Suggested change

	func (cs *canonicalStore[T]) deleteFrom(batch ethdb.Batch, fromPeriod uint64) (deleted Range) {
	keepRange, deleteRange := cs.periods.Split(fromPeriod)
	deleteRange.Each(func(period uint64) {
	batch.Delete(cs.databaseKey(period))
	cs.cache.Remove(period)
	})
	func (cs *canonicalStore[T]) deleteFrom(db ethdb.KeyValueWriter, fromPeriod uint64) (deleted Range) {
	keepRange, deleteRange := cs.periods.Split(fromPeriod)
	deleteRange.Each(func(period uint64) {
	db.Delete(cs.databaseKey(period))
	cs.cache.Remove(period)
	})

Let's use minimal interfaces

[zsfelfoldi]

done

[holiman]

Suggested change

	func newCanonicalStore[T any](db ethdb.KeyValueStore, keyPrefix []byte,
	func newCanonicalStore[T any](db ethdb.Iteratee, keyPrefix []byte,

[zsfelfoldi]

Done, though I'm not really sure whether it's really better to use different interfaces for the same backing database in different functions. I guess it's mostly a matter of taste, now at least we consistently use minimally required interfaces.

[holiman]

Suggested change

	func (cs *canonicalStore[T]) databaseKey(period uint64) []byte {
	var (
	kl = len(cs.keyPrefix)
	key = make([]byte, kl+8)
	)
	copy(key[:kl], cs.keyPrefix)
	binary.BigEndian.PutUint64(key[kl:], period)
	return key
	}
	func (cs *canonicalStore[T]) databaseKey(period uint64) []byte {
	return binary.BigEndian.AppendUint64(cs.keyPrefix, period)
	}

Isn't this equivalent?

[holiman]

Maybe this is safer

func (cs *canonicalStore[T]) databaseKey(period uint64) []byte {
	return binary.BigEndian.AppendUint64(append(nil, cs.keyPrefix...), period)
}

[zsfelfoldi]

I learned something today :) (never noticed that an AppendUint64 exists)

[holiman]

Must it be public? Please make it non-exported, for now at least

[zsfelfoldi]

done (I had to rename it to periodRange because range is a reserved word)

[holiman]

Also, a bit curious about this. We have a similar thing in ancients, where we have n 'tables', and during startup we sanity-check, and if any one table (e.g bodies) is shorter than the others, we need to trim the others so that they align.

In this code, you simply log a message and move on. What is the failure mode here? Is it an acceptable error if e.g. the updates are not aligned with the committee roots? Seems like we have three options

• As is: use what we have -- the range found, and move on
• Correlate: try to ensure that the three stores are aligned, if that's even applicable
• Error: raise a proper error to the calling code

[zsfelfoldi]

I went with the last option, there is no need to fix a faulty database here since it's easy enough to resync. This is also what checkConstraints does, the db is reset if it fails. Probably this is what would happen anyway if one of the stores were truncated. Now I return proper errors, show the error log and then reset the db just like in case of other inconsistencies.

[holiman]

Changes lgtm, but I have one remaining issue. I don't like on-the-fly encoders and decoders, in the canonicalStore. There are two possible fixes, as I see it.

1. Use rlp. for encoding. Then you can still let the objects be any. There's a bit of (arguably negligible) overhead, but rlp is the de-facto standard encoding in our codebase.
2. Use encoding.BinaryMarshaler and BinaryUnmarshaler. Then you would change the type from any into a combo-interface with those two.

[zsfelfoldi]

1. Use rlp. for encoding. Then you can still let the objects be any. There's a bit of (arguably negligible) overhead, but rlp is the de-facto standard encoding in our codebase.

Indeed, the overhead is very minimal, especially in absolute terms. Now it always uses rlp.

[holiman]

LGTM