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

Transaction sortition for inclusion in bundle #1530

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Transaction sortition for inclusion in bundle #1530

merged 1 commit into from
Jun 15, 2023

Conversation

rahulksnv
Copy link
Contributor

@rahulksnv rahulksnv commented Jun 10, 2023 •
edited
Loading

This PR implements the bundle proposer changes to include the transactions based on sortition. The tx range is static as of now. Dynamic adjustment of tx range would be in separate changes.

Code contributor checklist:

Issue: #1495

@rahulksnv rahulksnv requested a review from dariolina June 10, 2023 00:04
vedhavyas
vedhavyas reviewed Jun 12, 2023
View reviewed changes
Copy link
Member

@vedhavyas vedhavyas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

make sense over all. Is there any specific reasoning behind the initial tx_range ? If so, can you point me to the spec pls

Not a blocker but I feel Transaction selection makes more sense to me than transaction sortition and selector is used in some places as well.
Either is fine but we should be consistent

domains/client/domain-executor/src/sortition.rs Outdated Show resolved Hide resolved
dariolina
dariolina requested changes Jun 12, 2023
View reviewed changes
domains/client/domain-executor/src/bundle_election_solver.rs Outdated Show resolved Hide resolved
crates/sp-domains/src/lib.rs Outdated Show resolved Hide resolved
@rahulksnv
Copy link
Contributor Author

make sense over all. Is there any specific reasoning behind the initial tx_range ? If so, can you point me to the spec pls

Not a blocker but I feel Transaction selection makes more sense to me than transaction sortition and selector is used in some places as well. Either is fine but we should be consistent

Changed it (back to TxSelector, as it was originally). The cmd line flags are still call sortition, otherwise it becomes hard to tell this feature is sortition related

liuchengxu
liuchengxu reviewed Jun 13, 2023
View reviewed changes
Copy link
Contributor

@liuchengxu liuchengxu left a comment

Choose a reason for hiding this comment

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

A few nits and unsure about the feature flag enable_tx_sortition.

crates/subspace-node/src/core_domain/cli.rs Outdated Show resolved Hide resolved
domains/client/domain-executor/src/sortition.rs Outdated Show resolved Hide resolved
domains/client/domain-executor/src/sortition.rs Outdated Show resolved Hide resolved
domains/client/domain-executor/src/sortition.rs Outdated Show resolved Hide resolved
dariolina
dariolina reviewed Jun 13, 2023
View reviewed changes
Copy link
Member

@dariolina dariolina left a comment

Choose a reason for hiding this comment

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

Looks good to me logically. For the actual code quality others should approve

vedhavyas
vedhavyas previously approved these changes Jun 14, 2023
View reviewed changes
liuchengxu
liuchengxu reviewed Jun 14, 2023
View reviewed changes
Copy link
Contributor

@liuchengxu liuchengxu left a comment

Choose a reason for hiding this comment

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

Two minor comments and still unsure if the feature flag is necessary.

domains/client/domain-executor/src/domain_bundle_proposer.rs Outdated Show resolved Hide resolved
domains/client/domain-executor/Cargo.toml Outdated Show resolved Hide resolved
@rahulksnv
Copy link
Contributor Author

PTAL

@rahulksnv rahulksnv force-pushed the rsub/sort-a branch 4 times, most recently from bd54ee0 to 766954e Compare June 14, 2023 23:43
liuchengxu
liuchengxu previously approved these changes Jun 15, 2023
View reviewed changes
@rahulksnv @liuchengxu
Transaction sortition
c7689a2 
Co-authored-by: Liu-Cheng Xu <[email protected]>
@rahulksnv rahulksnv enabled auto-merge (squash) June 15, 2023 05:52
@rahulksnv rahulksnv merged commit 59b9d73 into main Jun 15, 2023
@rahulksnv rahulksnv deleted the rsub/sort-a branch June 15, 2023 06:09
@jfrank-summit jfrank-summit linked an issue Jun 20, 2023 that may be closed by this pull request
Domain bundle proposer changes for Txn sortition #1495
Closed
@jfrank-summit jfrank-summit mentioned this pull request Jun 20, 2023
Closed
i1i1 added a commit to autonomys/subspace-pulsar-sdk that referenced this pull request Jun 22, 2023
@i1i1
Import autonomys/subspace#1537,autonomys/subspace#1540,autonomys/subs…
76f5aac 
…pace#1538,autonomys/subspace#1540,autonomys/subspace#1541,autonomys/subspace#1543,autonomys/subspace#1530,autonomys/subspace#1548
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dariolina dariolina dariolina approved these changes

@liuchengxu liuchengxu liuchengxu left review comments

@vedhavyas vedhavyas vedhavyas left review comments

@nazar-pc nazar-pc Awaiting requested review from nazar-pc nazar-pc is a code owner

@rg3l3dr rg3l3dr Awaiting requested review from rg3l3dr rg3l3dr is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Domain bundle proposer changes for Txn sortition
4 participants
@rahulksnv @vedhavyas @liuchengxu @dariolina