feat: unsafe reset all #1196

piux2 · 2023-10-04T03:06:39Z

Contributors' checklist...

Added new tests
Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
Updated the official documentation
No breaking changes were made
Added references to related issues and PRs
Provided any useful hints for running manual tests
Added new benchmarks to generated graphs, if any. More info here.

The unsafe-reset-all is a method to hard reset the network to its genesis state. It's deemed unsafe and should only be used in a testing environment for several reasons:

It irreversibly deletes all node data, posing a significant risk of data loss.
There's a potential for double signing.
It resets the priv_validator_state.json, which could result in network disruptions and chain forks if not coordinated properly.

Additionally, in the PR, we made the following refactorings:

Removed the passing of os.Stdin wrapped in commands.IO to gno.land. Directly including os.Stdin in the command presents a vulnerability, as it isn't necessary for the program's main function. It's only required for testing purposes.
Introduced MockIO. This allows us to test with customized input strings, eliminating the need to pass os.Stdin directly to the node.
Stopped passing the config file via ff commands. The configuration required by ff demands that we expose all properties, not just a subset, for them to be defined in config.toml as flags in a CLI. This could result in more than 30 flags being listed. We now load the tm2 config separately. The current limitation is that we cannot overwrite it through flag options, but we plan to address this in a future update.

I understood we plan to have a gno sdk package. That is a bigger topic and many more aspects need to be considered. We can address it in a separate PR

codecov · 2023-10-04T03:10:34Z

Codecov Report

Attention: Patch coverage is 52.08333% with 92 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
gno.land/cmd/gnoland/reset.go	30.68%	57 Missing and 4 partials ⚠️
gno.land/cmd/gnoland/mockio.go	63.76%	18 Missing and 7 partials ⚠️
gno.land/cmd/gnoland/main.go	0.00%	6 Missing ⚠️

📢 Thoughts on this report? Let us know!

thehowl

What do you think is the advantage of unsafe-reset-all over make fclean? (See Makefile in gno.land), aside from the fact that it dynamically gets the db directory (which IIRC is not configurable at this time, anyway?).

I don't think we necessarily want/need to have this within the gno.land node code itself, but this is just an opinion.

thehowl · 2023-11-02T16:17:11Z

gno.land/cmd/gnoland/mockio.go

I'm against modifying global variables like os.Stdin for testing purposes. I think commands.IO works well, allows us for mock tests, and doesn't change anything in terms of security, on top of everything because this code is in a main package which cannot be imported.

Retaining os.Stdin wrapped in commands.IO within the gno.land runtime presents a security vulnerability. Here are the concerns.

Future Misuse: If future developers, unaware of the security implications, start utilizing commands.IO's os.Stdin to pass tests or for any other reasons. This could lead to vulnerabilities. Code that was originally not designed to interact with os.Stdin might be altered to do so, creating an attack vector.

Malicious IPC: A malicious entity could exploit this implementation to inject data into the gno.land node runtime through IPC ( Interprocess Communication)

Trust Concerns: Node operators could have trust concerns in the system's integrity, fearing potential injection attacks through IPC due to the presence of os.Stdin in the codebase.

While I agree with your argument, I believe it reinforces the original usage rather than the recent change. I see this change as a regression in terms of flexibility and security. Could you provide an example where your change enhances security?

Edit: my review covers this pull request: #1333, where the security and flexibility issues are clearer IMO. Please look at my latest comment here: https://github.com/gnolang/gno/pull/1196/files#r1385496885, where I suggest going back to the io.Commands approach but with a new read-only interface (without os.Stdin). This would help ensure readonly commands remain so by preventing accidental os.Stdin inputs.

thehowl · 2023-11-02T16:18:32Z

gno.land/cmd/gnoland/reset.go

+	blockdb := filepath.Join(dbDir, "blockstore.db")
+	state := filepath.Join(dbDir, "state.db")
+	wal := filepath.Join(dbDir, "cs.wal")
+	gnolang := filepath.Join(dbDir, "gnolang.db")
+
+	if osm.FileExists(blockdb) {
+		if err := os.RemoveAll(blockdb); err == nil {
+			logger.Info("Removed all blockstore.db", "dir", blockdb)
+		} else {
+			logger.Error("error removing all blockstore.db", "dir", blockdb, "err", err)
+		}


could be a for loop over an array/slice of strings instead of duplicating the same code 4 times...

also I think we can just do RemoveAll and log any error while returned by that function. 🤷

Good suggestion.

piux2 · 2023-11-02T19:58:02Z

What do you think is the advantage of unsafe-reset-all over make fclean? (See Makefile in gno.land), aside from the fact that it dynamically gets the db directory (which IIRC is not configurable at this time, anyway?).

I don't think we necessarily want/need to have this within the gno.land node code itself, but this is just an opinion.

unsafe-reset-all not only deletes the data store, it also resets the validator state to genesis state at zero height. In addition, we will need a reset state right after this, which will need to keep the validator state instead of reset to genesis state. make fclean will not be able to achieve it easily.

What are other places we might want to put the code in? It is in gno.land node code right now, because it for gno.land node operation, it does not server other purposes. I thought about Tm2. but it involves deleting the App DB, gnolang.db, which is for gnoland only.

moul · 2023-11-07T20:01:21Z

gno.land/cmd/gnoland/start_test.go

-			io.SetOut(commands.WriteNopCloser(mockOut))
-			io.SetErr(commands.WriteNopCloser(mockErr))
-			cmd := newRootCmd(io)
+		in, err := NewMockStdin(name)


I think this is a bad name, since we’re just trying to bufferize the stdout and not the stdin; we actually don’t care about stdin.

Name should be MockStdoutErr; or should just be MockIOs in general.

moul · 2023-11-07T20:01:42Z

gno.land/cmd/gnoland/start.go

@@ -158,10 +139,10 @@ func execStart(c *startCfg, args []string, io *commands.IO) error {
 		return fmt.Errorf("error in creating node: %w", err)
 	}

-	fmt.Fprintln(io.Err, "Node created.")
+	fmt.Println("Node created.")


Was better before.

moul · 2023-11-07T20:07:53Z

gno.land/cmd/gnoland/root.go

 )

-func main() {
-	io := commands.NewDefaultIO()


To retain the benefits of commands.IO while aligning with your approach, I propose introducing a commands.OutErr that disallows stdin setting and enables compiler verification.

If you're on board, we can proceed with my initial work to transition IO from a struct to an interface, detailed in this pull request: #1270.

Additionally, we can create a variant that functions as a "read-only IO," (commands.OutErr or better name) analogous to Golang's IO Writer, Reader, and ReadWriter, for commands that distinguish between 'out' and 'err.'

Do you intend commands.OutErr is an interface, subset of what would be commands.IO, which does not have Stdin?

moul · 2023-11-07T20:28:10Z

gno.land/cmd/gnoland/README.md

@@ -11,3 +11,16 @@
    $> gnoland start

 Afterward, you can interact with [`gnokey`](../gnokey) or launch a [`gnoweb`](../gnoweb) interface.
+
+
+## Reset `gnoland` node back to genesis state. It's only suitable for testnets.


Referring to #1201, to focus on a production-ready gnoland binary and maintain separate helper tools, we could simply add a contribs/gnoland-resetall, or a contribs/gnolandtools with an 'unsafe-reset-all' subcommand.

thehowl · 2024-11-27T17:55:48Z

Timed out; I think make fclean and gnogenesis can help us manipulate genesis well enough, and this PR would have to be seriously reworked.

feature: unsafe reset all

f952afb

piux2 requested a review from a team as a code owner October 4, 2023 03:06

github-actions bot assigned piux2 Oct 4, 2023

github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Oct 4, 2023

piux2 changed the title ~~feature: unsafe reset all~~ feat: unsafe reset all Oct 4, 2023

add seperate method

66e023f

thehowl reviewed Nov 2, 2023

View reviewed changes

piux2 mentioned this pull request Nov 2, 2023

Minutes: GitHub Review Meetings [every Thursday] gnolang/meetings#18

Open

3 tasks

moul reviewed Nov 7, 2023

View reviewed changes

thehowl closed this Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: unsafe reset all #1196

feat: unsafe reset all #1196

piux2 commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

thehowl left a comment

thehowl Nov 2, 2023

piux2 Nov 2, 2023

moul Nov 7, 2023 •

edited

Loading

thehowl Nov 2, 2023

piux2 Nov 3, 2023

piux2 commented Nov 2, 2023 •

edited

Loading

moul Nov 7, 2023

moul Nov 7, 2023

moul Nov 7, 2023

thehowl Nov 7, 2023

moul Nov 7, 2023

thehowl commented Nov 27, 2024

feat: unsafe reset all #1196

feat: unsafe reset all #1196

Conversation

piux2 commented Oct 4, 2023 • edited Loading

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

thehowl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moul Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piux2 commented Nov 2, 2023 • edited Loading

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

thehowl commented Nov 27, 2024

piux2 commented Oct 4, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading

moul Nov 7, 2023 •

edited

Loading

piux2 commented Nov 2, 2023 •

edited

Loading