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.

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

Add CreateSidechainProposal RPC, use ReverseHex for Txids #14

Merged
merged 3 commits into from
Oct 29, 2024
Merged

Add CreateSidechainProposal RPC, use ReverseHex for Txids #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3e39ada
Add CreateSidechainProposal RPC, use ReverseHex for Txids
Ash-L2L Oct 22, 2024
a5c78ea
stream (non-)confirmation events in CreateSidechainProposal response
Ash-L2L Oct 24, 2024
347003d
requested changes for PR 14
Ash-L2L Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion proto/cusf/common/v1/common.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,17 @@ message ConsensusHex {
/// Reverse-encoded hex
message ReverseHex {
google.protobuf.StringValue hex = 1;
}
}

message SidechainDeclarationV0 {
google.protobuf.StringValue title = 1;
google.protobuf.StringValue description = 2;
ConsensusHex hash_id_1 = 3;
ConsensusHex hash_id_2 = 4;
}

message SidechainDeclaration {
oneof version {
SidechainDeclarationV0 v0 = 1;
}
}
25 changes: 25 additions & 0 deletions proto/cusf/mainchain/v1/common.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/* Common message types */

syntax = "proto3";
package cusf.mainchain.v1;

import "google/protobuf/wrappers.proto";

import "cusf/common/v1/common.proto";

message OutPoint {
cusf.common.v1.ReverseHex txid = 1;
google.protobuf.UInt32Value vout = 2;
}

message SidechainDeclaration {
message V0 {
google.protobuf.StringValue title = 1;
google.protobuf.StringValue description = 2;
cusf.common.v1.ConsensusHex hash_id_1 = 3;
cusf.common.v1.ConsensusHex hash_id_2 = 4;
}
oneof sidechain_declaration {
V0 v0 = 1;
}
}
52 changes: 15 additions & 37 deletions proto/cusf/mainchain/v1/validator.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package cusf.mainchain.v1;
import "google/protobuf/wrappers.proto";

import "cusf/common/v1/common.proto";
import "cusf/mainchain/v1/common.proto";

message BlockHeaderInfo {
cusf.common.v1.ReverseHex block_hash = 1;
Expand All @@ -24,11 +25,6 @@ enum Network {
NETWORK_TESTNET = 5;
}

message OutPoint {
cusf.common.v1.ConsensusHex txid = 1;
uint32 vout = 2;
}

message Output {
cusf.common.v1.ConsensusHex address = 2;
uint64 value_sats = 3;
Expand Down Expand Up @@ -59,19 +55,6 @@ message BlockInfo {
optional cusf.common.v1.ConsensusHex bmm_commitment = 3;
}

message SidechainDeclarationV0 {
google.protobuf.StringValue title = 1;
google.protobuf.StringValue description = 2;
cusf.common.v1.ConsensusHex hash_id_1 = 3;
cusf.common.v1.ConsensusHex hash_id_2 = 4;
}

message SidechainDeclaration {
oneof version {
SidechainDeclarationV0 v0 = 1;
}
}

service ValidatorService {
rpc GetBlockHeaderInfo(GetBlockHeaderInfoRequest)
returns (GetBlockHeaderInfoResponse);
Expand Down Expand Up @@ -147,7 +130,7 @@ message GetCoinbasePSBTRequest {
}
message ProposeBundle {
google.protobuf.UInt32Value sidechain_number = 1;
cusf.common.v1.ConsensusHex bundle_txid = 2;
cusf.common.v1.ReverseHex bundle_txid = 2;
}
message AckBundles {
message RepeatPrevious {
Expand Down Expand Up @@ -178,7 +161,7 @@ message GetCtipRequest {
}
message GetCtipResponse {
message Ctip {
cusf.common.v1.ConsensusHex txid = 1;
cusf.common.v1.ReverseHex txid = 1;
uint32 vout = 2;
uint64 value = 3;
uint64 sequence_number = 4;
Expand All @@ -190,24 +173,19 @@ message GetSidechainProposalsRequest {
}
message GetSidechainProposalsResponse {
message SidechainProposal {
uint32 sidechain_number = 1;

// protolint:disable MAX_LINE_LENGTH

// Raw sidechain proposal data (M1 message of BIP300)
// https://github.com/bitcoin/bips/blob/master/bip-0300.mediawiki#m1----propose-sidechain
google.protobuf.BytesValue data = 2;
google.protobuf.UInt32Value sidechain_number = 1;

// protolint:enable MAX_LINE_LENGTH
// Raw sidechain proposal description
cusf.common.v1.ConsensusHex description = 2;

// Sidechain data, as declared in the M1 proposal.
// Might be nil, if the proposal uses an unknown version.
optional SidechainDeclaration declaration = 7;

cusf.common.v1.ConsensusHex data_hash = 3;
uint32 vote_count = 4;
uint32 proposal_height = 5;
uint32 proposal_age = 6;
cusf.common.v1.ReverseHex description_sha256d_hash = 3;
google.protobuf.UInt32Value vote_count = 4;
google.protobuf.UInt32Value proposal_height = 5;
google.protobuf.UInt32Value proposal_age = 6;
Comment on lines +186 to +188
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these wrappers? Same goes for line 176

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Primitive values have implicit presence in proto3. If they are set to the default value, they are not included in a serialized message, and if they are not included in a serialized message, they are assumed to have default value.

This is usually not what is desired - the consumer cannot distinguish between a missing field and the default value, so if a proto is updated to deprecate a field, the consumer will receive a default value instead of knowing that the value was not included.

Explicit field presence can be enabled with optional, or with google.protobuf.*Value messages. I dislike the use of optional to simply enable explicit field presence - optional is best used when a field is actually optional. This motivates the following convention:

  • Always use explicit field presence, unless the default value is always invalid and can be re-used to indicate field presence
  • Use google.protobuf.*Value messages to enable explicit field presence
  • Use optional to indicate that a field is optional

ie.

message Example {
    // Empty string is always invalid for an address, so this is fine
    string address = 1;
    // Empty string is always invalid for an address,
    // so this is ok to indicate an optional address
    optional string other_address = 2;
    // Zero amounts are valid, use explicit field presence
    google.protobuf.UInt32Value amount = 3;
    // Zero amounts are valid, and the value is optional,
    // so we can distinguish missing vs unspecified vs default
    optional google.protobuf.UInt32Value other_amount = 4;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I get your reasoning, and you have valid points. My opinion on this is that it introduces more complexity than is necessary. In my usage of Protobuf schemas over the years I've avoided using wrapper values and optionals for the vast, vast majority of requests/responses without issue. I think using raw values would work nicely here, and save us some extra wrapper usage.

I'll leave it up to you to decide!

}
repeated SidechainProposal sidechain_proposals = 1;
}
Expand All @@ -216,11 +194,11 @@ message GetSidechainsRequest {
}
message GetSidechainsResponse {
message SidechainInfo {
uint32 sidechain_number = 1;
google.protobuf.BytesValue data = 2;
uint32 vote_count = 3;
uint32 proposal_height = 4;
uint32 activation_height = 5;
google.protobuf.UInt32Value sidechain_number = 1;
cusf.common.v1.ConsensusHex description = 2;
google.protobuf.UInt32Value vote_count = 3;
google.protobuf.UInt32Value proposal_height = 4;
google.protobuf.UInt32Value activation_height = 5;
Comment on lines +197 to +201
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, I don't understand why these are wrappers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#14 (comment)

}
repeated SidechainInfo sidechains = 1;
}
Expand Down
37 changes: 35 additions & 2 deletions proto/cusf/mainchain/v1/wallet.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package cusf.mainchain.v1;
import "google/protobuf/wrappers.proto";

import "cusf/common/v1/common.proto";
import "cusf/mainchain/v1/common.proto";

service WalletService {
rpc BroadcastWithdrawalBundle(BroadcastWithdrawalBundleRequest)
Expand All @@ -16,6 +17,14 @@ service WalletService {
returns (CreateDepositTransactionResponse);
rpc CreateNewAddress(CreateNewAddressRequest)
returns (CreateNewAddressResponse);
// Create a new sidechain proposal (M1 in BIP300) and persist to the local
// database for further processing.
// Sidechain proposals must be included in the coinbase transaction of a
// newly mined block, so this proposal is not active until the wallet has
// been able to generate a new block.
// Returns a stream of (non-)confirmation events for the sidechain proposal.
rpc CreateSidechainProposal(CreateSidechainProposalRequest)
returns (stream CreateSidechainProposalResponse);
// Regtest only
rpc GenerateBlocks(GenerateBlocksRequest)
returns (GenerateBlocksResponse);
Expand All @@ -36,7 +45,7 @@ message CreateBmmCriticalDataTransactionRequest {
cusf.common.v1.ReverseHex prev_bytes = 5;
}
message CreateBmmCriticalDataTransactionResponse {
cusf.common.v1.ConsensusHex txid = 1;
cusf.common.v1.ReverseHex txid = 1;
}

message CreateDepositTransactionRequest {
Expand All @@ -46,7 +55,7 @@ message CreateDepositTransactionRequest {
uint64 fee_sats = 4;
}
message CreateDepositTransactionResponse {
cusf.common.v1.ConsensusHex txid = 1;
cusf.common.v1.ReverseHex txid = 1;
}

message CreateNewAddressRequest {
Expand All @@ -56,6 +65,30 @@ message CreateNewAddressResponse {
string address = 1;
}

message CreateSidechainProposalRequest {
google.protobuf.UInt32Value sidechain_id = 1;
SidechainDeclaration declaration = 2;
}

message CreateSidechainProposalResponse {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same Q goes here for wrappers

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

#14 (comment)

message Confirmed {
cusf.common.v1.ReverseHex block_hash = 1;
google.protobuf.UInt32Value confirmations = 2;
google.protobuf.UInt32Value height = 3;
OutPoint outpoint = 4;
cusf.common.v1.ReverseHex prev_block_hash = 5;
}
message NotConfirmed {
cusf.common.v1.ReverseHex block_hash = 1;
google.protobuf.UInt32Value height = 2;
cusf.common.v1.ReverseHex prev_block_hash = 3;
}
oneof event {
Confirmed confirmed = 1;
NotConfirmed not_confirmed = 2;
}
}

message GenerateBlocksRequest {
google.protobuf.UInt32Value blocks = 1;
}
Expand Down