Skip to content
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.

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

Functional tests for floresta-wire/sync-node #200

Merged
merged 1 commit into from
Aug 7, 2024

Conversation

lla-dane
Copy link
Contributor

Simulated a peer to take in requests for the sync-node instance. Filled the chain-state with some block-headers prior block validation and then send the appropriate responses to the node.

@lla-dane
Copy link
Contributor Author

lla-dane commented Jul 25, 2024

@Davidson-Souza, please suggest some more scenarios to test like you did for chain-selector, if needed.

@Davidson-Souza
Copy link
Collaborator

@Davidson-Souza, please suggest some more scenarios to test like you did for chain-selector, if needed.

The sync node is very simple, so I don't think there's much to test. But some things I can think of:

  • Test how it responds to a node sending a block without proof (this should ban the node)
  • How it responds to a block with invalid proof (this should ban the node, but should not invalidate the block)
  • Sending blocks out-of-order (it should handle them fine)
  • Sending duplicated blocks (this will increase the peer's ban score)

@Davidson-Souza
Copy link
Collaborator

Btw. The test_utils mod is the same code from #180 right? If you prefer, you can make one PR creating a module floresta-wire/src/p2p-wire/tests/util.rs and then rebase both this and #180 on that.

@lla-dane
Copy link
Contributor Author

Btw. The test_utils mod is the same code from #180 right? If you prefer, you can make one PR creating a module floresta-wire/src/p2p-wire/tests/util.rs and then rebase both this and #180 on that.

Sure, I will do that.

@lla-dane
Copy link
Contributor Author

The sync node is very simple, so I don't think there's much to test. But some things I can think of:

  • Test how it responds to a node sending a block without proof (this should ban the node)
  • How it responds to a block with invalid proof (this should ban the node, but should not invalidate the block)
  • Sending blocks out-of-order (it should handle them fine)

Added these tests, and fixed a bug along the way #203. After merge of #203, I will rebase this.
@Davidson-Souza please take a look, and suggest if I have to change something, then I will squash these commits.

@lla-dane lla-dane force-pushed the test/sync-node branch 3 times, most recently from 9436927 to dcc0888 Compare July 30, 2024 17:46
@lla-dane
Copy link
Contributor Author

phew, I guess I am done with this PR, please take a look @Davidson-Souza !!

Copy link
Collaborator

@Davidson-Souza Davidson-Souza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor things, in general looks great!

crates/floresta-wire/src/p2p_wire/mod.rs Show resolved Hide resolved
crates/floresta-wire/src/p2p_wire/tests/sync_node.rs Outdated Show resolved Hide resolved
@lla-dane lla-dane force-pushed the test/sync-node branch 2 times, most recently from e301255 to e1e4475 Compare August 7, 2024 13:51
@lla-dane
Copy link
Contributor Author

lla-dane commented Aug 7, 2024

@Davidson-Souza, please take a look, if this PR is ready for merge.

@Davidson-Souza Davidson-Souza merged commit 56dee17 into vinteumorg:master Aug 7, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants