Skip to content

Commit

Permalink
fix(ledger): backward merkle root chaining bugs (#347)
Browse files Browse the repository at this point in the history
## bug fixes
Previously, backwards merkle root chaining checks were:
- ignoring the actual result of the check
  - always marked it as a duplicate (as if the check failed)
  - always returned true (as if the check succeeded)
- comparing chained root to chained root, which is wrong. it should be chained to actual root

## refactoring
consolidate common chaining logic into single function "checkAndReportChaining"

Improve dependency management by:
- organizing merkle root check functions into a struct since they all share some common dependencies. this simplifies the function signatures and keeps things standardized and consistent
- utilizing working stores.
  - this is better than passing down the granular transitive dependencies (database and hashmap) separately because it clarifies what this code actually needs. all it really needs is some kind of store for erasure metas. it's not actually important in this context that it's managed through a negotiation between database and hashmap calls. this is the whole point of the pending state, to abstract away that negotiation. passing down all the transitive dependencies makes the function parameters a mess, and it becomes unclear why the code actually needs any of the dependencies.
  - this is better than passing down the whole pending state struct because it reduces the number of dependencies and it clarifies what is actually needed, rather than pretending the entire state is needed.

## change in behavior

merkle root checks now return an error if the merkle root is missing, instead of ignoring it, because that means there is a severe bug in the validator and it needs to be shutdown and patched immediately.
  • Loading branch information
dnut authored Nov 4, 2024
1 parent 7ad3927 commit 988fe6e
Show file tree
Hide file tree
Showing 6 changed files with 423 additions and 377 deletions.
34 changes: 21 additions & 13 deletions src/ledger/shred.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1048,16 +1048,20 @@ pub const ShredConstants = struct {
};

pub const layout = struct {
pub const SIZE_OF_COMMON_SHRED_HEADER: usize = 83;
pub const SIZE_OF_DATA_SHRED_HEADERS: usize = 88;
pub const SIZE_OF_CODE_SHRED_HEADERS: usize = 89;
pub const SIZE_OF_SIGNATURE: usize = sig.core.Signature.size;
pub const SIZE_OF_SHRED_VARIANT: usize = 1;
pub const SIZE_OF_SHRED_SLOT: usize = 8;

pub const OFFSET_OF_SHRED_VARIANT: usize = SIZE_OF_SIGNATURE; // 64
pub const OFFSET_OF_SHRED_SLOT: usize = SIZE_OF_SIGNATURE + SIZE_OF_SHRED_VARIANT; // 64 + 1 = 65
pub const OFFSET_OF_SHRED_INDEX: usize = OFFSET_OF_SHRED_SLOT + SIZE_OF_SHRED_SLOT; // 65 + 8 = 73
const SIZE_OF_COMMON_SHRED_HEADER: usize = 83;
const SIZE_OF_DATA_SHRED_HEADERS: usize = 88;
const SIZE_OF_CODE_SHRED_HEADERS: usize = 89;
const SIZE_OF_SIGNATURE: usize = sig.core.Signature.size;
const SIZE_OF_SHRED_VARIANT: usize = 1;
const SIZE_OF_SHRED_SLOT: usize = 8;
const SIZE_OF_INDEX: usize = 4;
const SIZE_OF_VERSION: usize = 2;

const OFFSET_OF_SHRED_VARIANT: usize = SIZE_OF_SIGNATURE; // 64
const OFFSET_OF_SHRED_SLOT: usize = SIZE_OF_SIGNATURE + SIZE_OF_SHRED_VARIANT; // 64 + 1 = 65
const OFFSET_OF_SHRED_INDEX: usize = OFFSET_OF_SHRED_SLOT + SIZE_OF_SHRED_SLOT; // 65 + 8 = 73
const OFFSET_OF_ERASURE_SET_INDEX: usize =
OFFSET_OF_SHRED_INDEX + SIZE_OF_INDEX + SIZE_OF_VERSION;

pub fn getShred(packet: *const Packet) ?[]const u8 {
if (getShredSize(packet) > packet.data.len) return null;
Expand Down Expand Up @@ -1096,7 +1100,7 @@ pub const layout = struct {
return Signature.init(shred[0..SIZE_OF_SIGNATURE].*);
}

pub fn getSignedData(shred: []const u8) ?Hash {
pub fn merkleRoot(shred: []const u8) ?Hash {
const variant = getShredVariant(shred) orelse return null;
const constants = switch (variant.shred_type) {
.code => code_shred_constants,
Expand All @@ -1105,6 +1109,10 @@ pub const layout = struct {
return getMerkleRoot(shred, constants, variant) catch null;
}

pub fn getErasureSetIndex(shred: []const u8) ?u32 {
return getInt(u32, shred, OFFSET_OF_ERASURE_SET_INDEX);
}

/// must be a data shred, otherwise the return value will be corrupted and meaningless
pub fn getParentSlotOffset(shred: []const u8) ?u16 {
std.debug.assert(getShredVariant(shred).?.shred_type == .data);
Expand Down Expand Up @@ -1213,8 +1221,8 @@ test "getLeaderSignature" {
try std.testing.expect(std.mem.eql(u8, &expected_signature, &signature.data));
}

test "getSignedData" {
const signed_data = layout.getSignedData(&test_data_shred).?;
test "layout.merkleRoot" {
const signed_data = layout.merkleRoot(&test_data_shred).?;
const expected_signed_data = [_]u8{
224, 241, 85, 253, 247, 62, 137, 179, 152, 192, 186, 203, 121, 194, 178, 130,
33, 181, 143, 156, 220, 150, 69, 197, 81, 97, 237, 11, 74, 156, 129, 134,
Expand Down
Loading

0 comments on commit 988fe6e

Please sign in to comment.