Bonds for Fortified DSMR #1835
Bonds for Fortified DSMR #1835
Conversation
joshua-kim
force-pushed
the
bond
branch
9 times, most recently
from
ae3d7a3 to
a41b0a8
Compare
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
| @@ -118,9 +126,9 @@ func (n *Node[T]) BuildChunk( | |||
| txs []T, | |||
| expiry int64, | |||
| beneficiary codec.Address, | |||
| ) (Chunk[T], ChunkCertificate, error) { | |||
There was a problem hiding this comment.
This was so gross previously (and I'm to blame...), we previously exposed the chunk/chunk certs that were created into the caller. Realistically this is just leaking implementation details because hypersdk would never care about the chunks that were built - since the mempool/consensus only cares about blocks.
There was a problem hiding this comment.
fwiw - I think it's fine to return these values or not export this function (could just expose Start(ctx, ...) that continuously builds chunks), but also fine to return the value if it makes writing tests easier
There was a problem hiding this comment.
The caller should only rely on some way to make sure chunks are continuously produced (assuming it's a validator node) until shutdown
|
|
||
| for _, chunk := range wantChunks { | ||
| found := false | ||
| for _, chunkCert := range blk.ChunkCerts { | ||
| if chunkCert.ChunkID == chunk.id { | ||
| found = true | ||
| } | ||
| } | ||
| r.True(found) | ||
| } |
There was a problem hiding this comment.
We don't check the chunk ids (and just chunk # of chunks now) because the caller shouldn't care what the chunk id is (as it just provides txs to build into a chunk).
| Height: parent.Height + 1, | ||
| Timestamp: parent.Timestamp + 1, | ||
| }, | ||
| ChunkCerts: parent.ChunkCerts, |
There was a problem hiding this comment.
Going over this - I think this is either weird/or a bug that we allow duplicate chunk certs to pass verification. For now i'm doing this because it passes tests and it's convenient - but probably something to fix as a follow-up.
There was a problem hiding this comment.
It's a bug fixed in #1810. We should not allow duplicates either in a block or across the ancestry.
| @@ -118,9 +126,9 @@ func (n *Node[T]) BuildChunk( | |||
| txs []T, | |||
| expiry int64, | |||
| beneficiary codec.Address, | |||
| ) (Chunk[T], ChunkCertificate, error) { | |||
There was a problem hiding this comment.
fwiw - I think it's fine to return these values or not export this function (could just expose Start(ctx, ...) that continuously builds chunks), but also fine to return the value if it makes writing tests easier
| @@ -118,9 +126,9 @@ func (n *Node[T]) BuildChunk( | |||
| txs []T, | |||
| expiry int64, | |||
| beneficiary codec.Address, | |||
| ) (Chunk[T], ChunkCertificate, error) { | |||
There was a problem hiding this comment.
The caller should only rely on some way to make sure chunks are continuously produced (assuming it's a validator node) until shutdown
x/dsmr/node.go
Outdated
| type Interface[T Tx] interface { | ||
| BuildChunk(ctx context.Context, txs []T, expiry int64, beneficiary codec.Address) error | ||
| BuildBlock(parent Block, timestamp int64) (Block, error) | ||
| Verify(ctx context.Context, parent Block, block Block) error | ||
| Accept(ctx context.Context, block Block) (ExecutedBlock[T], error) | ||
| } |
There was a problem hiding this comment.
What's the rationale for defining the interface this way? Asking because we've previously discussed removing defining interfaces over a single implementation
There was a problem hiding this comment.
I thought about this last night and originally felt this made sense so that we could keep wrapping dsmr with more features, but in hindsight maybe it makes more sense for this to live in the fdsmr package and say that dsmr implements it (i.e dsmr is fortifiable).
Another idea is to make fdsmr rely on the concrete type of dsmr since we only have one implementation but this makes testing look weirder.
x/fdsmr/node.go
Outdated
| if len(txs) == 0 { | ||
| return n.DSMR.BuildChunk(ctx, txs, expiry, beneficiary) | ||
| } | ||
|
|
There was a problem hiding this comment.
Should we remove this check since it will be handled the same way (slice of size 0 and skip the loop) regardless of if it's here?
There was a problem hiding this comment.
There is a very subtle difference... this is to handle this test case
Lines 36 to 45 in d1e726c
nil txs are wired into the underlying dsmr as nil as well (instead of a zero-length non-nil slice). This is probably overkill but I thought it made sense to be consistent w/ the caller value.
There was a problem hiding this comment.
I'd prefer to treat these the same and require the callee handles them the same as well instead of making sure to guarantee it here
x/fdsmr/node.go
Outdated
| if !b.pending.Contains(tx.GetID()) { | ||
| // this tx was either not assigned to us or was already un-bonded | ||
| return | ||
| } |
There was a problem hiding this comment.
When do we expect to hit this case?
There was a problem hiding this comment.
I might not understand something but this is because we call Unbond on every tx in the block and only call the underlying bonder if it was a tx that we knew about. I think this is needed because we're not guaranteed that every tx in a block was handled by us.
| Height: parent.Height + 1, | ||
| Timestamp: parent.Timestamp + 1, | ||
| }, | ||
| ChunkCerts: parent.ChunkCerts, |
There was a problem hiding this comment.
It's a bug fixed in #1810. We should not allow duplicates either in a block or across the ancestry.
x/fdsmr/node.go
Outdated
| if len(txs) == 0 { | ||
| return n.DSMR.BuildChunk(ctx, txs, expiry, beneficiary) | ||
| } | ||
|
|
There was a problem hiding this comment.
I'd prefer to treat these the same and require the callee handles them the same as well instead of making sure to guarantee it here
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
Signed-off-by: Joshua Kim <20001595+joshua-kim@users.noreply.github.com>
x/fdsmr/node.go
Outdated
| } | ||
| } | ||
|
|
||
| n.pending.SetMin(block.Timestamp) |
There was a problem hiding this comment.
SetMin returns the elements that were removed, could we use that instead of manually peeking/popping the elements to check if we need to call Unbond ?
There was a problem hiding this comment.
DIdn't know this return value existed, pretty nice!
| bonded = append(bonded, tx) | ||
| } | ||
|
|
||
| return n.DSMR.BuildChunk(ctx, bonded, expiry, beneficiary) |
There was a problem hiding this comment.
(not necessary for this PR) - is BuildChunk returning an error considered a fatal error? If not, we need to unbond any transactions that were bonded before building the chunk failed
Implements account bonding for DSMR. HyperSDK will implement the
Bonderinterface which will keep track of account bond balances.This PR includes a few changes to the
dsmrpackage to implement this, namely:dsmrandfdsmrare moved todsmrtestAcceptnow returns the outputted block with chunk details, because these details (i.e txs) are needed to unbond txs infdsmr.Acceptreturns the outputted block, we no longer needBuildChunkto return chunks/chunk certs to the caller, which was previously exposed for testing (bad practice).