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

Peer Management Enhancements (issue #488) #491

Merged
merged 43 commits into from
Jan 8, 2025
Merged

Peer Management Enhancements (issue #488) #491

merged 43 commits into from
Jan 8, 2025

Conversation

peterbroadhurst
Copy link
Contributor

@peterbroadhurst peterbroadhurst commented Dec 23, 2024

Implements #488

  • Update transport manager with new responsibilities
    • Reliable message DB tables
    • Peer management / activation
    • Peer inactivity reaping / deactivation
    • Fire+forget send short retry with Go channels
    • Reliable send from local DB tables, using sequence to track unsent messages
    • Ack/Nack sending
    • Ack/Nack storage
    • Move protox.Message to protox.PaladinMsg to remove some confusion in the code with gRPC Message
  • Reconciling private TX manager data duplicate data structures and redundant code
    • Reconcile components.TransportMessage and protox.PaladinMsg
    • Move list of Components to protox.PaladinMsg enum called Component
    • Change export/import for domain context to include un-written state data
      • Remove state distribution from Complete() on assemble, as states should flow in endorse payload
    • Update private TX manager to call new TransportManager APIs
  • Assembling / storing data types delivered via reliable message types
    • StateDistribution
      • Payload contains the domain/schema/id/nullifier info
      • State data itself is not duplicated in the DB, and only stored in the StateManager
    • Receipt
    • PreparedTransaction
  • JSON/RPC interface
    • transport_peers
    • transport_peerInfo
    • Add struct docs (and run gradle generateDocs)
    • Including statistics in the pldapi.PeerInfo structure
      type PeerInfo struct {
      Name string `docstruct:"PeerInfo" json:"name"`
      Stats PeerStats `docstruct:"PeerInfo" json:"stats"`
      Activated tktypes.Timestamp `docstruct:"PeerInfo" json:"activated"`
      Outbound map[string]any `docstruct:"PeerInfo" json:"outbound,omitempty"`
      OutboundError error `docstruct:"PeerInfo" json:"outboundError,omitempty"`
      }
      type PeerStats struct {
      SentMsgs uint64 `docstruct:"PeerStats" json:"sentMsgs"`
      ReceivedMsgs uint64 `docstruct:"PeerStats" json:"receivedMsgs"`
      SentBytes uint64 `docstruct:"PeerStats" json:"sentBytes"`
      ReceivedBytes uint64 `docstruct:"PeerStats" json:"receivedBytes"`
      LastSend *tktypes.Timestamp `docstruct:"PeerStats" json:"lastSend"`
      LastReceive *tktypes.Timestamp `docstruct:"PeerStats" json:"lastReceive"`
      ReliableHighestSent uint64 `docstruct:"PeerStats" json:"reliableHighestSent"`
      ReliableAckBase uint64 `docstruct:"PeerStats" json:"reliableAckBase"`
      }

Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst peterbroadhurst marked this pull request as ready for review January 6, 2025 03:00
@peterbroadhurst peterbroadhurst requested a review from hosie January 6, 2025 03:00
Copy link
Contributor

@hosie hosie left a comment

Choose a reason for hiding this comment

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

LGTM. A couple of minor comments but on the whole, this is a great improvement.

@@ -28,6 +28,8 @@ type TransportAPI interface {
ConfigureTransport(context.Context, *prototk.ConfigureTransportRequest) (*prototk.ConfigureTransportResponse, error)
SendMessage(context.Context, *prototk.SendMessageRequest) (*prototk.SendMessageResponse, error)
GetLocalDetails(context.Context, *prototk.GetLocalDetailsRequest) (*prototk.GetLocalDetailsResponse, error)
ActivateNode(context.Context, *prototk.ActivateNodeRequest) (*prototk.ActivateNodeResponse, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor spelling observation. ActivateNode could be misleading esp. if seen out of context. ActivatePeering might be more accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - went for ActivatePeer/DeactivatePeer

Signed-off-by: Peter Broadhurst <[email protected]>
@peterbroadhurst
Copy link
Contributor Author

Merging is dependent on #498, because of a small tweak to the prepare behavior in this PR - it now only sends the prepared transaction to the address designated as the from in that prepared transaction.

The E2E test / tutorial in example/bond is affected by this, but not after the refactor that was already in plan.

@awrichar and I discussed whether we should remove prepared transaction entirely in this PR, as they no longer have an active role, but we decided to decouple that change.

@peterbroadhurst
Copy link
Contributor Author

Hit intermittent build failure #410 - re-running

peterbroadhurst and others added 3 commits January 6, 2025 14:03
Fix the bond example to retrieve prepared transactions from the node that
prepared them, which may not be the node that requested them.

TODO: need to revisit if "prepare" is still a valid flow, as it's become complex

Signed-off-by: Andrew Richardson <[email protected]>
@peterbroadhurst
Copy link
Contributor Author

Thanks @awrichar for addressing the E2E

@peterbroadhurst peterbroadhurst merged commit 7d67e28 into main Jan 8, 2025
7 checks passed
@peterbroadhurst peterbroadhurst deleted the peers branch January 8, 2025 17:39
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.

3 participants