feat: create foundation for fraudulent block production #1992

evan-forbes · 2023-06-27T04:59:54Z

Overview

creates a malicious package that allows for minimal changes to the rest of the application while also keeping the malicious portion of the code as separate as possible as to not accidentally trigger the malicious logic.

part of #1953

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

evan-forbes · 2023-06-27T05:01:55Z

test/util/malicious/hasher.go

+	ErrInvalidNodeNamespaceOrder = errors.New("invalid NMT node namespace order")
+)
+
+type Hasher struct {


had to copy paste fork this from nmt in order to remove all of the protections against leafs that are out of order

As a note, it seems outside the scope of a hasher to have all these protections in place. That feels like it belongs to the jurisdiction of the tree itself

As a note, it seems outside the scope of a hasher to have all these protections in place. That feels like it belongs to the jurisdiction of the tree itself

Just a note on this, there are two hashers: one is the base hasher e.g., sha256, and one is the nmt hasher, or the namespace hasher, that utilizes the base hasher. The latter actually is aware of the format of the leaves and the nodes i.e., their min and max namespace, their ordering. Due to this, all the functions associated with the namespace hasher validate the format of inputs accordingly. However, the former, i.e., the basehasher, is completely unaware and is only responsible for the digest calculation. I am not sure, but I think you might be referring to the base-hasher, which, as you suggested, should be and is clueless about the namespace-related logic.

evan-forbes · 2023-06-27T05:02:57Z

test/util/malicious/test_app.go

+func NewAppServer(logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts servertypes.AppOptions) servertypes.Application {
+	var cache sdk.MultiStorePersistentCache


this function allows us to use the existing infrastructure (testnode, and even the create a new binary if need be), all while keeping the malicious code separate. Meaning, as long as we're using the normal application, we can't accidentally trigger the malicious logic.

evan-forbes · 2023-06-27T05:04:47Z

test/util/malicious/tree.go

+// NewTree creates a new rsmt2d.Tree using the malicious
+// wrapper.ErasuredNamespacedMerkleTree with predefined square size and
+// nmt.Options.
+func (c constructor) NewTree(_ rsmt2d.Axis, axisIndex uint) rsmt2d.Tree {
+	hasher := NewBlindHasher(appconsts.NewBaseHashFunc(), appconsts.NamespaceSize, true)
+	opts := []nmt.Option{
+		nmt.CustomHasher(hasher),
+		nmt.NamespaceIDSize(appconsts.NamespaceSize),
+		nmt.IgnoreMaxNamespace(true),
+	}
+	c.opts = append(c.opts, opts...)
+	nmtTree := nmt.New(appconsts.NewBaseHashFunc(), opts...)
+	maliciousTree := &BlindTree{nmtTree}
+	newTree := wrapper.NewErasuredNamespacedMerkleTree(c.squareSize, axisIndex, c.opts...)
+	newTree.SetTree(maliciousTree)
+	return &newTree
+}


apologies, I hate this too... we're wrapping the nmt wrapper to overwrite the push method and use our own malicious hasher

we can use this to commit to rows and cols that are out of lexicographical order

codecov-commenter · 2023-06-27T05:10:42Z

Codecov Report

Merging #1992 (a02c6e9) into main (ce0be95) will increase coverage by 0.54%.
The diff coverage is 36.77%.

❗ Current head a02c6e9 differs from pull request most recent head f4f88db. Consider uploading reports for the commit f4f88db to get more accurate results

@@            Coverage Diff             @@
##             main    #1992      +/-   ##
==========================================
+ Coverage   21.54%   22.08%   +0.54%     
==========================================
  Files         126      130       +4     
  Lines       14283    14479     +196     
==========================================
+ Hits         3077     3198     +121     
- Misses      10913    10981      +68     
- Partials      293      300       +7

Impacted Files	Coverage Δ
pkg/wrapper/nmt_wrapper.go	`80.00% <0.00%> (-3.34%)`	⬇️
test/util/testnode/config.go	`0.00% <0.00%> (ø)`
test/util/testnode/full_node.go	`0.00% <0.00%> (ø)`
test/util/testnode/node_interaction_api.go	`0.00% <0.00%> (ø)`
x/qgb/client/verify.go	`0.00% <0.00%> (ø)`
test/util/malicious/hasher.go	`51.37% <51.37%> (ø)`
test/util/malicious/test_app.go	`62.22% <62.22%> (ø)`
test/util/malicious/app.go	`85.00% <85.00%> (ø)`
test/util/malicious/tree.go	`100.00% <100.00%> (ø)`

cmwaters

This seems all good to me.

From a high level architecture perspective I see a bit of an anti-pattern here. For me, rsmt2d should just be a eds package within core-app. I don't know who else would want to use it and I'm not sure we should really look to cater to them over serving something purpose built for the celestia network. nmt is different because it's more self standing as it's own system whereas rsmt2d is kind glue between nmt, the square package and an erasure coding library. The fact that we have this wrapper package is a bit of a giveaway.

I would also prefer a design that has greater separation between the extended data square and the nmt that is built on top of this.

I've heard prior conversations about extracting out shares, square, blobs and eds into their own repo. This is something we could look to do

cmwaters · 2023-06-27T09:56:27Z

go.mod

@@ -3,7 +3,7 @@ module github.com/celestiaorg/celestia-app
 go 1.20

 require (
-	github.com/celestiaorg/nmt v0.16.0
+	github.com/celestiaorg/nmt v0.16.1-0.20230626222946-6dc07a6c7a35 // v0.16.0


since upgraded to v0.17.0

cmwaters · 2023-06-27T09:58:11Z

test/util/malicious/hasher.go

+	ErrInvalidNodeNamespaceOrder = errors.New("invalid NMT node namespace order")
+)
+
+type Hasher struct {


As a note, it seems outside the scope of a hasher to have all these protections in place. That feels like it belongs to the jurisdiction of the tree itself

cmwaters · 2023-06-27T10:01:05Z

test/util/malicious/app.go

@@ -0,0 +1,67 @@
+package malicious


nit: I'd prefer to call this package faulty for capturing general faulty behaviour but YMMV

rootulp

Overall LGTM

rootulp · 2023-06-27T14:59:37Z

go.mod

@@ -27,7 +27,7 @@ require (
 require (
 	cosmossdk.io/errors v1.0.0-beta.7
 	cosmossdk.io/math v1.0.0-rc.0
-	github.com/celestiaorg/rsmt2d v0.9.0
+	github.com/celestiaorg/rsmt2d v0.9.1-0.20230623202902-25e11e517e4c


[optional] should we cut an rsmt2d release so that this line can depend on an official release?

whoops, this doesn't actually require any changes to rsmt2d, this was just the recent codec changes, which we probably won't have to use

test/util/malicious/app.go

test/util/malicious/app_test.go

test/util/malicious/hasher.go

rootulp · 2023-06-27T15:11:00Z

test/util/malicious/hasher.go

+// Write writes the namespaced data to be hashed.
+//
+// Requires data of fixed size to match leaf or inner NMT nodes. Only a single
+// write is allowed.
+// It panics if more than one single write is attempted.
+// If the data does not match the format of an NMT non-leaf node or leaf node, an error will be returned.


[no change needed] maintainers should consider enabling a linter that auto wraps comment length

staheri14

LGTM!

test/util/malicious/hasher.go

staheri14 · 2023-06-27T20:03:21Z

test/util/malicious/hasher.go

+	ErrInvalidNodeNamespaceOrder = errors.New("invalid NMT node namespace order")
+)
+
+type Hasher struct {


As a note, it seems outside the scope of a hasher to have all these protections in place. That feels like it belongs to the jurisdiction of the tree itself

Just a note on this, there are two hashers: one is the base hasher e.g., sha256, and one is the nmt hasher, or the namespace hasher, that utilizes the base hasher. The latter actually is aware of the format of the leaves and the nodes i.e., their min and max namespace, their ordering. Due to this, all the functions associated with the namespace hasher validate the format of inputs accordingly. However, the former, i.e., the basehasher, is completely unaware and is only responsible for the digest calculation. I am not sure, but I think you might be referring to the base-hasher, which, as you suggested, should be and is clueless about the namespace-related logic.

The base branch was changed.

Co-authored-by: Rootul P <[email protected]>

evan-forbes · 2023-06-27T21:02:29Z

sorry for the force push, I had to rebase to stop github from showing the diff from #1991 along with this one 😑

evan-forbes · 2023-06-27T21:22:47Z

cc @cmwaters

From a high level architecture perspective I see a bit of an anti-pattern here. For me, rsmt2d should just be a eds package within core-app.

I agree completely! imo there are many potential ways we can organize or use the eds (one experiment from forever ago celestiaorg/rsmt2d#9 (comment)). wrapping and embedding all these different types and nmt.Visitor funcs feels kind of insane

ref celestiaorg/rsmt2d#10

staheri14

## Overview creates a malicious package that allows for minimal changes to the rest of the application while also keeping the malicious portion of the code as separate as possible as to not accidentally trigger the malicious logic. part of #1953 ## Checklist - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [x] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords --------- Co-authored-by: Rootul P <[email protected]>

This reverts commit 14c24b0.

evan-forbes requested a review from staheri14 June 27, 2023 04:59

evan-forbes self-assigned this Jun 27, 2023

evan-forbes requested review from rach-id, rootulp and cmwaters as code owners June 27, 2023 04:59

MSevey requested a review from a team June 27, 2023 05:00

evan-forbes changed the base branch from main to evan/refactor-testnode-appcreator June 27, 2023 05:00

evan-forbes commented Jun 27, 2023

View reviewed changes

cmwaters previously approved these changes Jun 27, 2023

View reviewed changes

MSevey requested a review from a team June 27, 2023 12:38

rootulp previously approved these changes Jun 27, 2023

View reviewed changes

staheri14 previously approved these changes Jun 27, 2023

View reviewed changes

Base automatically changed from evan/refactor-testnode-appcreator to main June 27, 2023 20:26

MSevey requested a review from a team June 27, 2023 20:50

evan-forbes and others added 9 commits June 27, 2023 15:57

feat!: add malicious application

2e222d3

feat: add the hasher

5ea91fd

feat: add malicious tree

693f5f2

chore: add quick test

a949d93

chore: linter

c6bade0

fix: options for constructor of blind tree

b056768

chore: bump to v0.17.0 of nmt

c261aea

chore: use v0.9.0 rsmt2d

dca34c2

fix: docs and feedback

f4f88db

Co-authored-by: Rootul P <[email protected]>

staheri14 previously approved these changes Jun 27, 2023

View reviewed changes

evan-forbes force-pushed the evan/bad-blocks branch from 461be6a to f4f88db Compare June 27, 2023 20:59

MSevey requested a review from a team June 27, 2023 20:59

chore: linter

1d6b2b0

evan-forbes dismissed staheri14’s stale review via 1d6b2b0 June 27, 2023 21:03

evan-forbes requested review from rootulp, cmwaters and staheri14 June 27, 2023 21:28

evan-forbes enabled auto-merge (squash) June 27, 2023 21:28

staheri14 approved these changes Jun 27, 2023

View reviewed changes

rootulp approved these changes Jun 28, 2023

View reviewed changes

evan-forbes merged commit 3aa3c09 into main Jun 28, 2023

evan-forbes deleted the evan/bad-blocks branch June 28, 2023 13:36

evan-forbes added a commit that referenced this pull request Jul 6, 2023

Revert "feat: create foundation for fraudulent block production (#1992)"

115a8be

This reverts commit 14c24b0.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create foundation for fraudulent block production #1992

feat: create foundation for fraudulent block production #1992

evan-forbes commented Jun 27, 2023

evan-forbes Jun 27, 2023

cmwaters Jun 27, 2023

staheri14 Jun 27, 2023

evan-forbes Jun 27, 2023 •

edited

Loading

evan-forbes Jun 27, 2023 •

edited

Loading

codecov-commenter commented Jun 27, 2023 •

edited

Loading

cmwaters left a comment

cmwaters Jun 27, 2023

evan-forbes Jun 27, 2023

cmwaters Jun 27, 2023

cmwaters Jun 27, 2023

rootulp left a comment

rootulp Jun 27, 2023

evan-forbes Jun 27, 2023

evan-forbes Jun 27, 2023

rootulp Jun 27, 2023

staheri14 left a comment

staheri14 Jun 27, 2023

evan-forbes commented Jun 27, 2023 •

edited

Loading

evan-forbes commented Jun 27, 2023

staheri14 left a comment

		func NewAppServer(logger log.Logger, db dbm.DB, traceStore io.Writer, appOpts servertypes.AppOptions) servertypes.Application {
		var cache sdk.MultiStorePersistentCache

feat: create foundation for fraudulent block production #1992

feat: create foundation for fraudulent block production #1992

Conversation

evan-forbes commented Jun 27, 2023

Overview

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

evan-forbes Jun 27, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Jun 27, 2023 • edited Loading

Codecov Report

cmwaters 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

rootulp 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

staheri14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Jun 27, 2023 • edited Loading

evan-forbes commented Jun 27, 2023

staheri14 left a comment

Choose a reason for hiding this comment

evan-forbes Jun 27, 2023 •

edited

Loading

evan-forbes Jun 27, 2023 •

edited

Loading

codecov-commenter commented Jun 27, 2023 •

edited

Loading

evan-forbes commented Jun 27, 2023 •

edited

Loading