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

initial version of cometbft consensus integration processing only assigned users/chains #24

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

ice-cronus
Copy link
Contributor

No description provided.

@ice-cronus ice-cronus changed the title initial version of cometbft consensus integration processing only igned users/chains initial version of cometbft consensus integration processing only assigned users/chains Nov 21, 2024
@ice-cronus ice-cronus marked this pull request as ready for review November 26, 2024 08:10
@ice-cronus ice-cronus requested a review from a team as a code owner November 26, 2024 08:10
@ice-cronus ice-cronus closed this Nov 26, 2024
@ice-cronus ice-cronus reopened this Nov 26, 2024
it := query.GetStoredEvents(ctx, &model.Subscription{Filters: model.Filters{
model.Filter{IDs: eTag},
}})
for ev := range it {
Copy link
Contributor

Choose a reason for hiding this comment

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

Error checking?

}
func mapTxToEvent(tx types.Tx) (*model.Event, error) {
var ev model.Event
if err := easyjson.Unmarshal(tx, &ev); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why easyjson?

require.NoError(t, repostEvent.Sign(nostr.GeneratePrivateKey()))
require.NoError(t, c.broadcastUserEvents(context.TODO(), repostEvent)())
})
query.MustInit(context.TODO())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move that query.MustInit() into TestMain?

@@ -153,8 +179,8 @@ on conflict do update set
tags = excluded.tags,
d_tag = excluded.d_tag,
reference_id = excluded.reference_id,
hidden = 0
`
hidden = %[1]v
Copy link
Contributor

Choose a reason for hiding this comment

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

Keep that stmt as const and use excluded.hidden here.

t.Run("query events", func(t *testing.T) {
events, err := relay.QuerySync(ctx, nostr.Filter{Kinds: []int{nostr.KindTextNote}})
if err != nil {
log.Panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

require.NoError(t, err)

@@ -80,6 +82,7 @@ func TestMain(m *testing.M) {
serverCancel()

if code == 0 {
time.Sleep(10 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

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

why 10 ? :)

)

func newConsensus() *consensus {
return mustInit(context.Background()).(*consensus)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test context, with timeout

func AcceptEvents(ctx context.Context, events ...*model.Event) error {
return globalDB.Client.AcceptEvents(ctx, events...)
func AcceptEvents(ctx context.Context, consenusAccepted bool, events ...*model.Event) error {
return globalDB.Client.AcceptEvents(ctx, consenusAccepted, events...)
Copy link
Contributor

Choose a reason for hiding this comment

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

no, don't do that;

add a new func RollbackAcceptedEvents(ctx context.Context, events ...*model.Event) error { that u call to rollback

return errors.Wrapf(err, "failed to command.AcceptEvent(%#v)", events)
}
if err := query.AcceptEvents(ctx, events...); err != nil {
command.RegisterConsensusEventListener(func(ctx context.Context, events ...*model.Event) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do u have 2 EventListeners?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

second is when consensus accepted, we need it to accept events broadcasted by other nodes and currently it is used to mark as consensus accepted on current / initial node as well

for chainID, mxNode := range globalNodes {
mxnode := (mxNode.GetInstance().(*node.Node))
broadcasters[chainID] = multiplex.NewChainInstance(chainID, local.New(mxnode))
go func(chain string, n *node.Node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

u no longer need that; its been fixed in https://tip.golang.org/doc/go1.22


if w.Len() > 0 {
w.WriteString(" AND ")
if !w.IncludeHidden {
Copy link
Contributor

Choose a reason for hiding this comment

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

a better idea to try to generate a delete logic based on the events, so u can rollback the inserts, would be to have a mapping between a requestID and the resulted rows inserted

so that you can rollback by simply delete by pk directly; or insert by sql directly

U modify the normal flow to save the results (RETURNING *) of the sql in a sync.Map

go.mod Outdated
@@ -1,6 +1,6 @@
module github.com/ice-blockchain/subzero

go 1.23.3
go 1.23.2
Copy link
Contributor

Choose a reason for hiding this comment

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

rollback

@ice-cronus ice-cronus force-pushed the feature/cometbft_consensus branch from b1348b1 to 835ba63 Compare December 11, 2024 06:44
@ice-cronus ice-cronus force-pushed the feature/cometbft_consensus branch from 835ba63 to 60efbb3 Compare December 18, 2024 12:09
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