initial rough commit of a work/attestation pool #46

tersec · 2018-12-21T20:15:46Z

Tests will come later, but built to be testable, a separate abstract data type which can be plugged in.

Data structures aren't great, but they're internal and subject to change later.

arnetheduck

without updating the bitfield, this branch isn't that useful - also, pushed some bit setters in #47 that might help set the right bits, while we wait for the spec to settle.. later, we'll probably move to nim-ranges and the bit array type there

arnetheduck · 2018-12-21T22:40:10Z

beacon_chain/work_pool.nim

+
+  # TODO: What's the best way to union participation_bitfield as seq[byte]?
+  # Obvious methods, but a reusable abstraction probably exists.
+  # result.participation_bitfield =


well, without updating this, it won't pass validation.. easiest union is just a for loop

Sure, absolutely, both points. It's just that said for loop ideally doesn't belong here, ideally. It's what obvious methods refers to. I'll implement it and per your other comment, move to nim-ranges later.

Something in the style of toOpenArray(cast[ptr byte](foo), 0, foo.len - 1)

@arnetheduck added one for-loop approach to do this. I'd prefer something more functional or at least immutable, but doing that, retaining compositionality, and not creating extra memory allocations requires more machinery than seems warranted for a hopefully temporary hack.

@mratsim will address that in an upcoming commit.

if we can stay away from cast, that's generally a bonus

Yeah, I'm leaving this as a for loop + nim-ranges TODO for now. Open to alternatives.

arnetheduck · 2018-12-22T00:43:52Z

beacon_chain/work_pool.nim

+  result.attestations = initTable[AttestationData, seq[Attestation]]()
+
+func getLookupKey(attestationData: AttestationData): array[0..31, byte] =
+  hash_tree_root(attestationData)


there's hash_tree_final after #47 is merged, which gives you an eth2digest..

Sounds good, I'll wait until #47 is merged and use hash_tree_final.

another todo: garbage collection when enough slot time has passed

Noted.

arnetheduck · 2018-12-22T00:47:17Z

beacon_chain/work_pool.nim

+    signatures.add(perShardAttestation.aggregate_signature)
+  combine(signatures)
+
+func getAggregatedAttestion*(pool: AttestationPool, shard: uint64) : Attestation =


might make more sense to just aggregate directly in the pool, dropping any attestations that are already part of the key - not sure if there's ever interest to keep non-aggregated attestations around.. since you can't use the same pubkey twice, my guess would be that the easiest thing for clients to do is to publish attestations with only one signature and have everyone do the aggregation locally (instead of trying to publish aggregate sigs) - later this can be refined.

I'd prefer this design too, but that's what the comment

# # Per Danny as of 2018-12-21: # Yeah, you can do any linear combination of signatures. but you have to # remember the linear combination of pubkeys that constructed # if you have two instances of a signature from pubkey p, then you need 2*p # in the group pubkey because the attestation bitfield is only 1 bit per # pubkey right now, attestations do not support this it could be extended to # support N overlaps up to N times per pubkey if we had N bits per validator # instead of 1 # We are shying away from this for the time being. If there end up being # substantial difficulties in network layer aggregation, then adding bits to # aid in supporting overlaps is one potential solution

in the beginning effectively is supposed to address. The code I had and threw away did something more like what you describe -- aggregate incrementally, track who's included, all that.

Currently, I guess we wouldn't see the same pubkey twice, with just gossip pub/sub broadcasting, but that's not specified one way or the other in the protocol and explicitly still up in the air. So I'm wary of designing the attestation/work pool to rely on that kind of quirk too tightly.

But maybe it's safe enough.

well, on the gossip network I'd expect only single-signature attestations to travel (for now) - so it probably makes sense here that the code checks any incoming attetation for overlap and that's it - if you save every attestation and try to find the best coverage every time a new one comes in, that's a lot of work as well..

point is, I'm a validator, I know which block I'm responsible for, so I start collecting data (shard state, beacon state, pow state), and when the moment is right, I publish an attestation to the latest data that I know, signed only by me, without considering the attestation pool at all.. that's the simplest correct strategy - later maybe we can play with others.

hm, wonder what the economy of that is - if everyone drops attestations that cover more than the client already has, it means you're aligned to send attestations with few signatures in the hope of getting them accepted in the pool - the counterbalance is that when you're a proposer, you want to have as many attestations as possible.. hm.

anyway.

Yeah, this seems reasonable, and works better in other ways. Done in new branch.

The cryptoeconomic/game theory/etc would need to get better thought out, but that's not my initial goal.

mratsim

The type and/or init looks wrong

mratsim · 2018-12-22T10:16:15Z

beacon_chain/work_pool.nim

+  # It would be better to combine these incrementally, pending above.
+  # This back-loads the work.
+  AttestationPool* = object
+    attestations: Table[uint64, Table[array[32, byte], seq[Attestation]]]


For my need this can just be Table[AttesterIdx, ref seq[AttestationData]].

THe uint64 should be aliased to make the purpose clear.

When consolidating/combining incrementally, this particular organization ends up making less sense regardless.

mratsim · 2018-12-22T10:16:41Z

beacon_chain/work_pool.nim

+  # per shard
+
+proc init*(T: type AttestationPool): T =
+  result.attestations = initTable[AttestationData, seq[Attestation]]()


Inconsistent with type declaration?

Yes, fixed in new branch, and added smoke test to keep it working. Also simplified type at cost of retrieval, but good tradeoff right now.

mratsim · 2018-12-22T10:17:36Z

beacon_chain/work_pool.nim

+func getLookupKey(attestationData: AttestationData): array[0..31, byte] =
+  hash_tree_root(attestationData)
+
+proc addAttestation*(pool: var AttestationPool,


This should be called add to avoid attestationPool.addAttestation and be consistent with nim seqs.

mratsim · 2018-12-22T10:19:57Z

beacon_chain/work_pool.nim

+
+  # TODO: What's the best way to union participation_bitfield as seq[byte]?
+  # Obvious methods, but a reusable abstraction probably exists.
+  # result.participation_bitfield =


Something in the style of toOpenArray(cast[ptr byte](foo), 0, foo.len - 1)

…document future improvments from spec update #47

arnetheduck · 2018-12-24T12:53:57Z

another todo: garbage collection when enough slot time has passed

* Move initAPI to newDispatcher() call.

initial rough commit of a work/attestation pool

09a92ed

tersec requested review from arnetheduck and mratsim December 21, 2018 20:15

arnetheduck requested changes Dec 22, 2018

View reviewed changes

mratsim reviewed Dec 22, 2018

View reviewed changes

tersec added 2 commits December 22, 2018 02:24

add kludgy participation_bitfield handling, some error handling, and …

7880b51

…document future improvments from spec update #47

rename addAttestation to add for Nim consistency

25dfe95

mratsim merged commit 6864451 into master Dec 26, 2018

tersec deleted the InitialAttestationPool branch December 28, 2018 17:10

This was referenced Jan 2, 2019

Address comments from #46 #49

Closed

some implementation of getLatestAttestationTarget and some nascient testing #56

Closed

etan-status pushed a commit that referenced this pull request May 12, 2022

Fix emscripten compilation errors. (#46)

b41af14

* Move initAPI to newDispatcher() call.

etan-status pushed a commit that referenced this pull request May 12, 2022

Fix behavior which was agreed in #46.

b0fe839

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial rough commit of a work/attestation pool #46

initial rough commit of a work/attestation pool #46

tersec commented Dec 21, 2018

arnetheduck left a comment

arnetheduck Dec 21, 2018

tersec Dec 22, 2018

mratsim Dec 22, 2018

tersec Dec 22, 2018

arnetheduck Dec 24, 2018

tersec Jan 2, 2019

arnetheduck Dec 22, 2018

tersec Dec 22, 2018

tersec Jan 2, 2019

arnetheduck Dec 22, 2018

tersec Dec 22, 2018

arnetheduck Dec 24, 2018 •

edited

arnetheduck Dec 24, 2018

tersec Jan 2, 2019

mratsim left a comment

mratsim Dec 22, 2018

tersec Jan 2, 2019

mratsim Dec 22, 2018

tersec Jan 2, 2019

mratsim Dec 22, 2018

tersec Dec 22, 2018

mratsim Dec 22, 2018

arnetheduck commented Dec 24, 2018

initial rough commit of a work/attestation pool #46

initial rough commit of a work/attestation pool #46

Conversation

tersec commented Dec 21, 2018

arnetheduck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnetheduck Dec 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mratsim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnetheduck commented Dec 24, 2018

arnetheduck Dec 24, 2018 •

edited