feat: add a transaction mirror binary #7183

marcelo-gonzalez · 2022-07-13T18:31:18Z

This adds code that mirrors traffic from a source chain (e.g. mainnet
or testnet) to a test chain with genesis state forked from the source
chain. The goal is to produce traffic that looks like source chain
traffic. So in a mocknet test where we fork mainnet state for example,
we can then actually observe what happens when we subsequently get
traffic equivalent to mainnet traffic after the fork point. For more
info, see the README in this commit.

This adds code that mirrors traffic from a source chain (e.g. mainnet or testnet) to a test chain with genesis state forked from the source chain. The goal is to produce traffic that looks like source chain traffic. So in a mocknet test where we fork mainnet state for example, we can then actually observe what happens when we subsequently get traffic equivalent to mainnet traffic after the fork point. For more info, see the README in this commit.

marcelo-gonzalez · 2022-07-13T18:35:13Z

@nikurt @mina86 it's kinda long so no rush on the review.... to check if it works, I think its helpful to compile https://github.com/marcelo-gonzalez/nearcore/tree/txs, and then run

$ cd pytest
$ python3 ./tests/mirror/test.py
$ neard-txs --home ~/.near/mirror/source view-state txs --start 1 --end 200
$ neard-txs --home ~/.near/mirror/target view-state txs --start 1 --end 200

to compare the traffic between the two chains

nikurt

The PR is complicated, but nothing seems to be wrong.

pytest/tests/mirror/test.py

tools/mirror/src/chain_tracker.rs

tools/mirror/src/key_mapping.rs

nikurt · 2022-07-14T09:53:31Z

tools/mirror/src/lib.rs

+            }
+            Action::DeleteKey(delete_key) => {
+                let replacement =
+                    crate::key_mapping::map_key(&delete_key.public_key, self.secret.as_ref());


I don't understand how this is supposed to work.
If an account has 10 keys, and 1 gets deleted. Will this result in deleting shardnet's secret key, even though the account has 9 other keys? Each of the remaining keys is also mapped to the same shardnet's secret key.

Each of the 10 keys will be mapped to a different shardnet secret key.

Like if account foo.near has 2 keys A="ed25519:He7QeRu..." and B="ed25519:3KyUucjyG...", and say map_key(A).public()="ed25519:FXXrTXi..." and map_key(B).public() = "ed25519:Eo9W44tRM..." (it doesnt actually, I made up those values), then if the source chain has a DeleteKey("ed25519:3KyUucjyG...") tx, we will generate a DeleteKey("ed25519:Eo9W44tRM...") tx for the target chain

mina86

Looked at Python part so far; will take a look at the rest later today.

pytest/tests/mirror/test.py

mina86

Looks sensible to me though this PR suffers from severe lack of comments.

pytest/tests/mirror/test.py

tools/mirror/src/chain_tracker.rs

mina86 · 2022-07-14T21:09:50Z

tools/mirror/src/chain_tracker.rs

+
+    fn remove_tx(&mut self, tx: &IndexerTransactionWithOutcome) {
+        let k = (tx.transaction.signer_id.clone(), tx.transaction.public_key.clone());
+        match self.txs_by_signer.entry(k.clone()) {


This clone is not necessary because you can use e.key() instead of &k (and thus you don’t need to hold onto k).

ah but then we have a mutable borrow and a regular borrow out at the same time :/

tools/mirror/src/lib.rs

…e value already exists

…), not just public_key

marcelo-gonzalez · 2022-07-19T22:18:06Z

Looks sensible to me though this PR suffers from severe lack of comments.

Ahh yea I'm always too light on the comments... :/ Btw @mina86, wonder if you have thoughts about what to do about the fact that the pytest in this PR is not included as a nightly test (which is why the presubmit fails). The test doesnt actually have any real automated correctness assertions, and you can only tell if things are okay by manually inspecting the DBs. Could argue that then that means I should add some asserts and make a real test, but even still don't really want to add noise with this test if it's failing for some reason.

this is actually pretty important

don't remove the nonce info from the DB on DeleteKey actions. This does nothing but make our txs wrong, since at the moment of preparing this DeleteKey action, it's of course not on chain yet, so we still want the nonce info around

the source nonce should be set to what it is after the AddKey action is applied, not after the chunks in first block using it are applied. The increment to NonceDiff::target_start is actually an unnecessary thing that masks this bug, and when there are 2 txs using the added key in the first block that uses it, one of them will fail because of this

the system.run() is not what we want because nobody calls stop()

the previous logic made us send txs out of order (which is prob not a big deal, but is kind of unclean and could actually matter even if it's low probability), and it looked up access keys over and over inefficiently

marcelo-gonzalez · 2022-10-19T17:08:26Z

ok there were a bunch of bugs that took a while longer to fix than I thought. added a bunch of commits just now that should fix them, fyi @nikurt in case something looks wrong to you and you wanna revoke the LGTM. but basically all the commits added are bug fixes (just that they required a lot of changes to fix properly), and the basic functionality is the same as before.

one bug still left is that when there are accounts added with the create_account method on the near contract, those accounts' transactions wont get sent to the target test chain. will send a fix for that later, but could maybe just manually rewrite the methods in all of those function call transactions to have the mapped keys in the account_id field. Or if it's worth doing and we wanna get fancy maybe we could modify all contract code so that calls to promise_batch_action_add_key_* are replaced with calls to a wrapper function that maps the key and then calls that. not sure if that's worth doing but if it's not too bad that would be the way to go I think

mina86 · 2022-10-20T01:23:41Z

Btw @mina86, wonder if you have thoughts about what to do about the fact that the pytest in this PR is not included as a nightly test (which is why the presubmit fails).

So the trivial answer is to add it to whitelist in scripts/check_pytest.py. However, I think putting non-nightly tests in pytest/test is a bad practice and I would instead prefer if all of them ended up in some other place. Maybe pytest/scripts or pytest/tools.

marcelo-gonzalez · 2022-10-20T02:07:15Z

@matklad I remember you mentioned that adding more binaries increases build times. Did you mean when people just run cargo build w/ no arguments, or some build in a script somewhere? I was about to add another binary here in this commit, and I don't think it would make a lot of sense to add this to neard (nobody is gonna want this). But maybe it's possible to add this binary but exclude it somehow in Cargo.toml (w/ default-members or something?) ?

matklad · 2022-10-20T10:51:14Z

I remember you mentioned that adding more binaries increases build times. Did you mean when people just run cargo build w/ no arguments, or some build in a script somewhere?

Yeah, cargo build & cargo test. We don't want to exclude the code by default: we still want to typecheck it everytime to make sure it doesn't bitrot. What we can reasonably avoid is linking step: if you have one nearcore library, and n biaries using the lib, then the lib is typechecked only once, but it is linked n times.

That's not a super strict rule -- it's not like something breaks if we have too many binaries, but overall "do not add extra bins" is a good rule of thumb.

It also helps with maintenence a bit: it's much harder to "forget" a binary if it is a part of neard --help.

nobody is gonna want this

So this is the core thing it seems. we want to use this, while validators generally don't. I thin the best pattern here is some kind of subcommand namespace for internal subcommands: neard x view-state, neard x ping, neard x mirror-net, where x is documented as not having any kind of compatability or security guarantees and, maybe, even is excluded from prod builds.

As a user of internal tools, I'd love to know that ./neard x --help is the only command I need to know about to discover everything, as opposed to looking through the tools directory and figuring out which tool is a real binary tool, and which is just some random piece of code.

No strong opinion here, and it's on node team to make a value judgement here!

marcelo-gonzalez · 2022-10-21T14:50:11Z

ok moved it to a neard subcommand. @matklad would you mind taking a quick look at how i did that? (the last commit bff7e6c)

matklad

👍

This adds code that mirrors traffic from a source chain (e.g. mainnet or testnet) to a test chain with genesis state forked from the source chain. The goal is to produce traffic that looks like source chain traffic. So in a mocknet test where we fork mainnet state for example, we can then actually observe what happens when we subsequently get traffic equivalent to mainnet traffic after the fork point. For more info, see the README in this commit.

marcelo-gonzalez requested a review from a team as a code owner July 13, 2022 18:31

marcelo-gonzalez requested review from nikurt and mina86 July 13, 2022 18:31

nikurt approved these changes Jul 14, 2022

View reviewed changes

mina86 reviewed Jul 14, 2022

View reviewed changes

marcelo-gonzalez added 9 commits July 14, 2022 10:50

fix indentation

55b9200

add comments

7dae99d

extract ordinal_to_port() function

624aecd

use subprocess.check_output()

60ce7c5

use pathlib mkdir()

5db97a9

use json.dump()

bd5c603

kill process if cant stop with SIGINT

4c20b36

assert exit code is 0

2c0ada6

fix typo

e485416

mina86 reviewed Jul 14, 2022

View reviewed changes

marcelo-gonzalez added 4 commits July 19, 2022 15:27

simplify stuff

e7881b5

fix bug. Don't overwrite the nonces column with a default value if th…

6fc1a36

…e value already exists

dont fetch nonces we already know about

5b9a42b

fix bug. the Nonces column should be keyed by (account_id, public_key…

237e87c

…), not just public_key

marcelo-gonzalez added 8 commits July 27, 2022 14:10

take care of the TODO saying not to hardcode the sleep time

4dfb256

this is actually pretty important

Merge branch 'master' into mirror

dc7e081

adjust

eb39d0a

fix DeleteKey bug.

d4644a3

don't remove the nonce info from the DB on DeleteKey actions. This does nothing but make our txs wrong, since at the moment of preparing this DeleteKey action, it's of course not on chain yet, so we still want the nonce info around

handle implicit accounts

1b41393

add --no-secret flag

90b0c13

fix up error handling and actix logic

01347ba

the system.run() is not what we want because nobody calls stop()

marcelo-gonzalez added 4 commits October 19, 2022 12:51

fix up logic on setting nonces

7686196

the previous logic made us send txs out of order (which is prob not a big deal, but is kind of unclean and could actually matter even if it's low probability), and it looked up access keys over and over inefficiently

remove txs from sent_txs if we know theyre skipped

7c91a8a

remove redundant log. already logged in on_txs_sent()

3de06ab

cleanup pytest

0d95f98

marcelo-gonzalez added 2 commits October 19, 2022 21:39

change pytest dir to test-mirror

67a492a

move pytest/tests/mirror to pytest/tools/mirror

87f1de3

marcelo-gonzalez added 4 commits October 20, 2022 12:26

remove parity-secp256k1 dep

0a462df

Merge remote-tracking branch 'origin/master' into mirror

aa2070c

fix up Cargo.toml

1589e07

move it to a neard subcommand

bff7e6c

dont start tokio for the prepare command

a9b067e

matklad reviewed Oct 21, 2022

View reviewed changes

marcelo-gonzalez added the S-automerge label Oct 21, 2022

near-bulldozer bot merged commit 8e11368 into near:master Oct 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add a transaction mirror binary #7183

feat: add a transaction mirror binary #7183

marcelo-gonzalez commented Jul 13, 2022

marcelo-gonzalez commented Jul 13, 2022

nikurt left a comment

nikurt Jul 14, 2022

marcelo-gonzalez Jul 14, 2022

mina86 left a comment

mina86 left a comment

mina86 Jul 14, 2022

marcelo-gonzalez Jul 19, 2022

marcelo-gonzalez commented Jul 19, 2022

marcelo-gonzalez commented Oct 19, 2022

mina86 commented Oct 20, 2022

marcelo-gonzalez commented Oct 20, 2022

matklad commented Oct 20, 2022

marcelo-gonzalez commented Oct 21, 2022

matklad left a comment

feat: add a transaction mirror binary #7183

feat: add a transaction mirror binary #7183

Conversation

marcelo-gonzalez commented Jul 13, 2022

marcelo-gonzalez commented Jul 13, 2022

nikurt left a comment

Choose a reason for hiding this comment

nikurt Jul 14, 2022

Choose a reason for hiding this comment

marcelo-gonzalez Jul 14, 2022

Choose a reason for hiding this comment

mina86 left a comment

Choose a reason for hiding this comment

mina86 left a comment

Choose a reason for hiding this comment

mina86 Jul 14, 2022

Choose a reason for hiding this comment

marcelo-gonzalez Jul 19, 2022

Choose a reason for hiding this comment

marcelo-gonzalez commented Jul 19, 2022

marcelo-gonzalez commented Oct 19, 2022

mina86 commented Oct 20, 2022

marcelo-gonzalez commented Oct 20, 2022

matklad commented Oct 20, 2022

marcelo-gonzalez commented Oct 21, 2022

matklad left a comment

Choose a reason for hiding this comment