-
Notifications
You must be signed in to change notification settings - Fork 667
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
Feat/stackerdb messages #3551
Feat/stackerdb messages #3551
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3551 +/- ##
===========================================
- Coverage 0.18% 0.18% -0.01%
===========================================
Files 306 306
Lines 278640 280746 +2106
===========================================
Hits 512 512
- Misses 278128 280234 +2106
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…he consensus hash of the first sortition in that reward cycle
… stackerdb protocol, and have remote peers reply to handshakes with a list of stackerdbs that they support (which is then tracked in the conversation)
Can you add some rustdocs that provide an overview of what this new protocol is for, how it should work, and how it fits into the rest of the networking code? Otherwise, this is pretty difficult to provide a thorough review. |
Yes; I just added some in c2d3ebd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me -- I just had some feedback about comments, and then about the ContractId
wrapper struct (I think should be replaced with an extension trait).
…lias QualifiedContractIdentifier to ContractId
…etwork/stacks-blockchain into feat/stackerdb-messages
@kantai Thanks for your review! All of your comments have been addressed. Please let me know what you think 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
) -> Result<Option<ReplyHandleP2P>, net_error> { | ||
assert!(preamble.payload_len > 1); // don't count 1-byte type prefix | ||
|
||
if !self.process_relayers(local_peer, preamble, &relayers) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back at this PR now that it's active again -- is there test coverage for the error branches here? (either this invalid relayers branch or the bandwidth exceeded branch)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The process_relayers()
function is tested in the test convo_process_relayers()
already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that be sufficient for merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine for process_relayers()
, but there's many error cases in this changeset:
if self.connection.options.max_stackerdb_push_bandwidth > 0
&& self.stats.get_stackerdb_push_bandwidth()
> (self.connection.options.max_stackerdb_push_bandwidth as f64)
StacksMessageType::StackerDBHandshakeAccept(ref data, ref db_data) => {
if solicited {
test_debug!("{:?}: Got unauthenticated StackerDBHandshakeAccept", &self);
self.handle_handshake_accept(burnchain_view, &msg.preamble, data, Some(db_data))
.and_then(|_| Ok(None))
} else {
test_debug!(
"{:?}: Unsolicited unauthenticated StackerDBHandshakeAccept",
&self
);
// don't update stats or state, and don't pass back
consume = true;
Ok(None)
}
match self.validate_stackerdb_push(
local_peer,
chain_view,
&msg.preamble,
msg.relayers.clone(),
)? {
Some(handle) => Ok(handle),
None => {
// will forward upstream
return Ok(Some(msg));
}
}
And for this branch, are both conditions tested (i.e., node supports "stacker dbs", but not the same stacker db as the handshake initializer?)
let stacks_message = if ConversationP2P::supports_stackerdb(local_peer.services)
&& ConversationP2P::supports_stackerdb(self.peer_services)
{
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure; added a bunch of missing test coverage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, aside from those minor changes I suggested in the documentation, and assuming some tests will get added in src/net/stackerdb/tests/mod.rs in future PRs.
Co-authored-by: Brice Dobry <[email protected]>
Co-authored-by: Brice Dobry <[email protected]>
Co-authored-by: Brice Dobry <[email protected]>
Okay, with the latest tests, this looks good to me. Before merging, be sure that you want this in |
This PR implements the message codes and handshake protocol for nodes that support Stacker DBs. This is part of an effort to split #3534 into a series of smaller PRs.