Skip to content

Commit 3d96e3c

Browse files
rauljordanrauljordan
authored
Amend DepositNumberAndRootAtHeight Function to Fix Off-By-One (#8809)
* amend deposit cache and add comprehensive unit tests to fix off-by-one error * fix tests * still need to +1 deposit index
1 parent 8cda50f commit 3d96e3c

File tree

2 files changed

+142
-116
lines changed

2 files changed

+142
-116
lines changed

‎beacon-chain/cache/depositcache/deposits_cache.go

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -167,20 +167,19 @@ func (dc *DepositCache) AllDeposits(ctx context.Context, untilBlk *big.Int) []*e
167167
return deposits
168168
}
169169

170-
// DepositsNumberAndRootAtHeight returns number of deposits made prior to blockheight and the
171-
// root that corresponds to the latest deposit at that blockheight.
170+
// DepositsNumberAndRootAtHeight returns the number of deposits that exist at a specified
171+
// block height. If no deposits are found at that block height, we return the number of deposits
172+
// at the first block height we find right beneath it. If nothing is found at all, we return 0 and
173+
// the empty root.
172174
func (dc *DepositCache) DepositsNumberAndRootAtHeight(ctx context.Context, blockHeight *big.Int) (uint64, [32]byte) {
173-
ctx, span := trace.StartSpan(ctx, "DepositsCache.DepositsNumberAndRootAtHeight")
174-
defer span.End()
175175
dc.depositsLock.RLock()
176176
defer dc.depositsLock.RUnlock()
177-
heightIdx := sort.Search(len(dc.deposits), func(i int) bool { return dc.deposits[i].Eth1BlockHeight > blockHeight.Uint64() })
178-
// send the deposit root of the empty trie, if eth1follow distance is greater than the time of the earliest
179-
// deposit.
180-
if heightIdx == 0 {
181-
return 0, [32]byte{}
177+
for i := len(dc.deposits) - 1; i >= 0; i-- {
178+
if dc.deposits[i].Eth1BlockHeight <= blockHeight.Uint64() {
179+
return uint64(dc.deposits[i].Index) + 1, bytesutil.ToBytes32(dc.deposits[i].DepositRoot)
180+
}
182181
}
183-
return uint64(heightIdx), bytesutil.ToBytes32(dc.deposits[heightIdx-1].DepositRoot)
182+
return 0, params.BeaconConfig().ZeroHash
184183
}
185184

186185
// DepositByPubkey looks through historical deposits and finds one which contains

‎beacon-chain/cache/depositcache/deposits_cache_test.go

Lines changed: 133 additions & 106 deletions
Original file line numberDiff line numberDiff line change
@@ -154,121 +154,148 @@ func TestAllDeposits_FiltersDepositUpToAndIncludingBlockNumber(t *testing.T) {
154154
assert.Equal(t, 5, len(d))
155155
}
156156

157-
func TestDepositsNumberAndRootAtHeight_ReturnsAppropriateCountAndRoot(t *testing.T) {
158-
dc, err := New()
159-
require.NoError(t, err)
160-
161-
dc.deposits = []*dbpb.DepositContainer{
162-
{
163-
Eth1BlockHeight: 10,
164-
Deposit: &ethpb.Deposit{
165-
Data: &ethpb.Deposit_Data{
166-
PublicKey: make([]byte, 48),
167-
WithdrawalCredentials: make([]byte, 32),
168-
Signature: make([]byte, 96),
169-
},
157+
func TestDepositsNumberAndRootAtHeight(t *testing.T) {
158+
wantedRoot := bytesutil.PadTo([]byte("root"), 32)
159+
t.Run("requesting_last_item_works", func(t *testing.T) {
160+
dc, err := New()
161+
require.NoError(t, err)
162+
dc.deposits = []*dbpb.DepositContainer{
163+
{
164+
Eth1BlockHeight: 10,
165+
Index: 0,
166+
Deposit: &ethpb.Deposit{},
170167
},
171-
},
172-
{
173-
Eth1BlockHeight: 10,
174-
Deposit: &ethpb.Deposit{
175-
Data: &ethpb.Deposit_Data{
176-
PublicKey: make([]byte, 48),
177-
WithdrawalCredentials: make([]byte, 32),
178-
Signature: make([]byte, 96),
179-
},
168+
{
169+
Eth1BlockHeight: 10,
170+
Index: 1,
171+
Deposit: &ethpb.Deposit{},
180172
},
181-
},
182-
{
183-
Eth1BlockHeight: 10,
184-
Deposit: &ethpb.Deposit{
185-
Data: &ethpb.Deposit_Data{
186-
PublicKey: make([]byte, 48),
187-
WithdrawalCredentials: make([]byte, 32),
188-
Signature: make([]byte, 96),
189-
},
173+
{
174+
Eth1BlockHeight: 11,
175+
Index: 2,
176+
Deposit: &ethpb.Deposit{},
190177
},
191-
},
192-
{
193-
Eth1BlockHeight: 10,
194-
Deposit: &ethpb.Deposit{
195-
Data: &ethpb.Deposit_Data{
196-
PublicKey: make([]byte, 48),
197-
WithdrawalCredentials: make([]byte, 32),
198-
Signature: make([]byte, 96),
199-
},
178+
{
179+
Eth1BlockHeight: 13,
180+
Index: 3,
181+
Deposit: &ethpb.Deposit{},
182+
DepositRoot: wantedRoot,
200183
},
201-
},
202-
{
203-
Eth1BlockHeight: 11,
204-
Deposit: &ethpb.Deposit{
205-
Data: &ethpb.Deposit_Data{
206-
PublicKey: make([]byte, 48),
207-
WithdrawalCredentials: make([]byte, 32),
208-
Signature: make([]byte, 96),
209-
},
184+
}
185+
n, root := dc.DepositsNumberAndRootAtHeight(context.Background(), big.NewInt(13))
186+
assert.Equal(t, 4, int(n))
187+
require.DeepEqual(t, wantedRoot, root[:])
188+
})
189+
t.Run("only_one_item", func(t *testing.T) {
190+
dc, err := New()
191+
require.NoError(t, err)
192+
193+
dc.deposits = []*dbpb.DepositContainer{
194+
{
195+
Eth1BlockHeight: 10,
196+
Index: 0,
197+
Deposit: &ethpb.Deposit{},
198+
DepositRoot: wantedRoot,
210199
},
211-
DepositRoot: bytesutil.PadTo([]byte("root"), 32),
212-
},
213-
{
214-
Eth1BlockHeight: 12,
215-
Deposit: &ethpb.Deposit{
216-
Data: &ethpb.Deposit_Data{
217-
PublicKey: make([]byte, 48),
218-
WithdrawalCredentials: make([]byte, 32),
219-
Signature: make([]byte, 96),
220-
},
200+
}
201+
n, root := dc.DepositsNumberAndRootAtHeight(context.Background(), big.NewInt(10))
202+
assert.Equal(t, 1, int(n))
203+
require.DeepEqual(t, wantedRoot, root[:])
204+
})
205+
t.Run("none_at_height_some_below", func(t *testing.T) {
206+
dc, err := New()
207+
require.NoError(t, err)
208+
209+
dc.deposits = []*dbpb.DepositContainer{
210+
{
211+
Eth1BlockHeight: 8,
212+
Index: 0,
213+
Deposit: &ethpb.Deposit{},
221214
},
222-
},
223-
{
224-
Eth1BlockHeight: 12,
225-
Deposit: &ethpb.Deposit{
226-
Data: &ethpb.Deposit_Data{
227-
PublicKey: make([]byte, 48),
228-
WithdrawalCredentials: make([]byte, 32),
229-
Signature: make([]byte, 96),
230-
},
215+
{
216+
Eth1BlockHeight: 9,
217+
Index: 1,
218+
Deposit: &ethpb.Deposit{},
219+
DepositRoot: wantedRoot,
231220
},
232-
},
233-
}
234-
235-
n, root := dc.DepositsNumberAndRootAtHeight(context.Background(), big.NewInt(11))
236-
assert.Equal(t, 5, int(n))
237-
assert.Equal(t, bytesutil.ToBytes32([]byte("root")), root)
238-
}
239-
240-
func TestDepositsNumberAndRootAtHeight_ReturnsEmptyTrieIfBlockHeightLessThanOldestDeposit(t *testing.T) {
241-
dc, err := New()
242-
require.NoError(t, err)
243-
244-
dc.deposits = []*dbpb.DepositContainer{
245-
{
246-
Eth1BlockHeight: 10,
247-
Deposit: &ethpb.Deposit{
248-
Data: &ethpb.Deposit_Data{
249-
PublicKey: make([]byte, 48),
250-
WithdrawalCredentials: make([]byte, 32),
251-
Signature: make([]byte, 96),
252-
},
221+
{
222+
Eth1BlockHeight: 11,
223+
Index: 2,
224+
Deposit: &ethpb.Deposit{},
253225
},
254-
DepositRoot: bytesutil.PadTo([]byte("root"), 32),
255-
},
256-
{
257-
Eth1BlockHeight: 11,
258-
Deposit: &ethpb.Deposit{
259-
Data: &ethpb.Deposit_Data{
260-
PublicKey: make([]byte, 48),
261-
WithdrawalCredentials: make([]byte, 32),
262-
Signature: make([]byte, 96),
263-
},
226+
}
227+
n, root := dc.DepositsNumberAndRootAtHeight(context.Background(), big.NewInt(10))
228+
assert.Equal(t, 2, int(n))
229+
require.DeepEqual(t, wantedRoot, root[:])
230+
})
231+
t.Run("none_at_height_none_below", func(t *testing.T) {
232+
dc, err := New()
233+
require.NoError(t, err)
234+
235+
dc.deposits = []*dbpb.DepositContainer{
236+
{
237+
Eth1BlockHeight: 8,
238+
Index: 0,
239+
Deposit: &ethpb.Deposit{},
240+
DepositRoot: wantedRoot,
264241
},
265-
DepositRoot: bytesutil.PadTo([]byte("root"), 32),
266-
},
267-
}
268-
269-
n, root := dc.DepositsNumberAndRootAtHeight(context.Background(), big.NewInt(2))
270-
assert.Equal(t, 0, int(n))
271-
assert.Equal(t, [32]byte{}, root)
242+
}
243+
n, root := dc.DepositsNumberAndRootAtHeight(context.Background(), big.NewInt(7))
244+
assert.Equal(t, 0, int(n))
245+
require.DeepEqual(t, params.BeaconConfig().ZeroHash, root)
246+
})
247+
t.Run("none_at_height_one_below", func(t *testing.T) {
248+
dc, err := New()
249+
require.NoError(t, err)
250+
251+
dc.deposits = []*dbpb.DepositContainer{
252+
{
253+
Eth1BlockHeight: 8,
254+
Index: 0,
255+
Deposit: &ethpb.Deposit{},
256+
DepositRoot: wantedRoot,
257+
},
258+
}
259+
n, root := dc.DepositsNumberAndRootAtHeight(context.Background(), big.NewInt(10))
260+
assert.Equal(t, 1, int(n))
261+
require.DeepEqual(t, wantedRoot, root[:])
262+
})
263+
t.Run("some_greater_some_lower", func(t *testing.T) {
264+
dc, err := New()
265+
require.NoError(t, err)
266+
267+
dc.deposits = []*dbpb.DepositContainer{
268+
{
269+
Eth1BlockHeight: 8,
270+
Index: 0,
271+
Deposit: &ethpb.Deposit{},
272+
},
273+
{
274+
Eth1BlockHeight: 8,
275+
Index: 1,
276+
Deposit: &ethpb.Deposit{},
277+
},
278+
{
279+
Eth1BlockHeight: 9,
280+
Index: 2,
281+
Deposit: &ethpb.Deposit{},
282+
DepositRoot: wantedRoot,
283+
},
284+
{
285+
Eth1BlockHeight: 10,
286+
Index: 3,
287+
Deposit: &ethpb.Deposit{},
288+
},
289+
{
290+
Eth1BlockHeight: 10,
291+
Index: 4,
292+
Deposit: &ethpb.Deposit{},
293+
},
294+
}
295+
n, root := dc.DepositsNumberAndRootAtHeight(context.Background(), big.NewInt(9))
296+
assert.Equal(t, 3, int(n))
297+
require.DeepEqual(t, wantedRoot, root[:])
298+
})
272299
}
273300

274301
func TestDepositByPubkey_ReturnsFirstMatchingDeposit(t *testing.T) {

0 commit comments

Comments
 (0)