Skip to content

Invalid TX corrupts the local node's blockchain state #70

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
EnchanterIO opened this issue Feb 6, 2019 · 4 comments
Closed

Invalid TX corrupts the local node's blockchain state #70

EnchanterIO opened this issue Feb 6, 2019 · 4 comments
Assignees
@ggarri @EnchanterIO
Labels
blocked Issue can't be resolved due to a unresolved dependency bug Something isn't working

Comments

@EnchanterIO
Copy link
Contributor

EnchanterIO commented Feb 6, 2019 •
edited by ggarri
Loading

Steps to reproduce:

  1. Submit a valid TX (none 1)
  2. Submit an invalid TX (e.g. too big of a size > 32kb) (nonce 2)
  3. Submit a valid TX (nonce 3), this TX will fail due to a nonce mismatch in a state (nonce 2)

Additional details

The above behaviour is due to different states being maintained across the application. In this case, the issue is TX Pool forcing a new nonce and not unregistering invalid TXs that didn't get through.

How to reproduce

  • Run a node
  • Execute the workload truffle test and wait for first txs to be broadcasted
truffle test ./test/04_workload.js --network=sirius
  • Shut down now in the middle of the test execution
  • Wait for few minutes and boot the node again.
  • Every new txs signed by the same account used to performance the test will provide the following error output
ERROR[02-27|15:16:05.557] Nonce not strictly increasing. Expected 6926 got 6928 engine=consensus module=ABCI

Solution

  • Ensure an invalid TX doesn't get added to the pool
  • If that happens because we have more validation logic on ABCI::checkTx() side than Ethereum in TxPool::validateTx(), remove the TX from the pool or reset the nonce.

Note: I will first submit a PR cleaning the codebase, adding doc adding more logging etc and then a separate PR with a fix
@EnchanterIO EnchanterIO self-assigned this Feb 6, 2019
@EnchanterIO EnchanterIO added the bug Something isn't working label Feb 6, 2019
@EnchanterIO EnchanterIO added this to the Stable Joe milestone Feb 6, 2019
EnchanterIO added a commit that referenced this issue Feb 11, 2019
@EnchanterIO
#70 Cleans up consensus and database pkg. Adds docs and better logging.
e7b4e4f
EnchanterIO added a commit that referenced this issue Feb 11, 2019
@EnchanterIO
#70 Increases nonce in a local separate checkTxState as it was before.
2e11bef
EnchanterIO added a commit that referenced this issue Feb 11, 2019
@EnchanterIO
#70 Renames ExecuteTx to DeliverTx that IDE accidentally refactored i…
5f6e7f0 
…n docs.
EnchanterIO added a commit that referenced this issue Feb 11, 2019
@EnchanterIO
#70 Make it more clear where LastCommitInfo is coming from in docs.
cf694bf
EnchanterIO added a commit that referenced this issue Feb 11, 2019
@EnchanterIO
#70 Moves TendermintABCI to its own package and creates a Struct per …
a1e0aef 
…connection.
EnchanterIO added a commit that referenced this issue Feb 11, 2019
@EnchanterIO
#70 Makes the ABCI connections unexported.
85ebb4d
ggarri pushed a commit that referenced this issue Feb 12, 2019
@EnchanterIO @ggarri
#70 Cleans up consensus and database pkg. Adds docs and better logging.
25b66f9
ggarri pushed a commit that referenced this issue Feb 12, 2019
@EnchanterIO @ggarri
#70 Increases nonce in a local separate checkTxState as it was before.
2d5c099
ggarri pushed a commit that referenced this issue Feb 12, 2019
@EnchanterIO @ggarri
#70 Renames ExecuteTx to DeliverTx that IDE accidentally refactored i…
020e81a 
…n docs.
ggarri pushed a commit that referenced this issue Feb 12, 2019
@EnchanterIO @ggarri
#70 Make it more clear where LastCommitInfo is coming from in docs.
8ba91dc
ggarri pushed a commit that referenced this issue Feb 12, 2019
@EnchanterIO @ggarri
#70 Moves TendermintABCI to its own package and creates a Struct per …
0808c15 
…connection.
ggarri pushed a commit that referenced this issue Feb 12, 2019
@EnchanterIO @ggarri
#70 Makes the ABCI connections unexported.
5e1a4b9
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Replaces reduntant commit and call to get block state.
ae5f80d
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Replaces reduntant commit and call to get block state.
90e951e
@EnchanterIO
Copy link
Contributor Author

EnchanterIO commented Feb 12, 2019 •
edited
Loading

Update:

The above commits were attempting to bring more encapsulation to the ABCI package and while it looks like it was successful, after more thought I came to conclusion it wasn't. Writing object oriented code in GoLang while using pointers looks visually better, more comprehensive but it couples things even more because it pushes the shared state, dependency, behind more abstraction which only hides the problem under the carpet.

A different approach is needed where the ABCI dependencies and all the states are properly decoupled from the legacy codebase. In order to do so, we should first have solid tests covering the bases to ensure the changes behave as expected. I will close this task by only documenting the ABCI and refactoring naming conventions across the ABCI implementation but won't introduce more changes till tests are in place.

A new PR from branch: feature/70-consensus_db_pkgs_cleanup_no_breaking_changes will be created in few mins.

EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Documents ABCI, merges validateTx() to checkTx() and order methods.
6995954
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Extracts workstate to its own file, blockstate.go
c1d069c
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Renames workState to blockState and cleans its methods, adds docs.
bf8a631
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Removes db.ethState setters in favour of constructor args.
1b04ad6
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Renames ethState::commit() to persist() as it writes to disk and …
6454ab2 
…is not related to ABCI.
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Renames DB::commit() to DB::persist() as it writes to disk and is…
ef8d5c3 
… not related to ABCI.
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Renames BlockProcessor to BlockValidator... because it's not a pr…
05de81d 
…ocessor!

There is another Struct called BlockProcessor that is completely different.
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Documents ABCI, merges validateTx() to checkTx() and order methods.
cd4ac73
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Extracts workstate to its own file, blockstate.go
c376305
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Renames workState to blockState and cleans its methods, adds docs.
ea009a7
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Removes db.ethState setters in favour of constructor args.
11f7c7d
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Renames ethState::commit() to persist() as it writes to disk and …
c558946 
…is not related to ABCI.
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Renames DB::commit() to DB::persist() as it writes to disk and is…
c77c9ec 
… not related to ABCI.
EnchanterIO added a commit that referenced this issue Feb 12, 2019
@EnchanterIO
#70 Renames BlockProcessor to BlockValidator... because it's not a pr…
f5ef491 
…ocessor!

There is another Struct called BlockProcessor that is completely different.
@EnchanterIO EnchanterIO added the blocked Issue can't be resolved due to a unresolved dependency label Feb 12, 2019
@EnchanterIO
Copy link
Contributor Author

Blocked until we have integration tests proving the issue and validating rest of Lightchain functionalities work in the current moment as expected. Blocked by: #76

EnchanterIO added a commit that referenced this issue Feb 27, 2019
@EnchanterIO
#70 Adds task ID to Todo comments.
802bf83
EnchanterIO added a commit that referenced this issue Feb 27, 2019
@EnchanterIO
#79 Prevents lightchain node dir to be overwritten accidentally on in…
c195bde 
…it. (#88)

* #79 Prevents lightchain node dir to be overwritten accidentally on init.

* #70 Adds task ID to Todo comments.
ggarri added a commit that referenced this issue Feb 27, 2019
@ggarri
#70 Adding documentation to work around corrupted local state
9f153a3
EnchanterIO pushed a commit that referenced this issue Feb 27, 2019
@ggarri @EnchanterIO
#70 Adding troubleshooting doc to work around corrupted local state (#91
e32c7b0 
)
EnchanterIO pushed a commit that referenced this issue Mar 5, 2019
@ggarri @EnchanterIO
#70 Adding troubleshooting doc to work around corrupted local state (#91
8f23457 
)
@EnchanterIO EnchanterIO removed this from the Stable Joe milestone Mar 5, 2019
@EnchanterIO
Copy link
Contributor Author

Tracing is down, now blocked till all the absolutely necessary mainet tasks are finished and then we fix the sad path that corrupts local account's nonce. If this happens, here are instructions how to easily fix the problem: https://github.com/lightstreams-network/lightchain/#troubleshooting

ggarri added a commit that referenced this issue Mar 25, 2019
@ggarri
#70 Forcing tx to run as remote
00acd81
EnchanterIO pushed a commit that referenced this issue Mar 25, 2019
@ggarri @EnchanterIO
#70 Forcing tx to run as remote
0042c87
@ggarri ggarri self-assigned this Mar 25, 2019
@ggarri
Copy link
Contributor

ggarri commented May 27, 2019

Fixed on last commit 0042c87

@ggarri ggarri closed this as completed May 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ggarri ggarri

@EnchanterIO EnchanterIO

Labels
blocked Issue can't be resolved due to a unresolved dependency bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@ggarri @EnchanterIO