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

cuprated: txpool #312

Merged
merged 22 commits into from
Oct 29, 2024
Merged

cuprated: txpool #312

merged 22 commits into from
Oct 29, 2024

Conversation

Boog900
Copy link
Member

@Boog900 Boog900 commented Oct 13, 2024

What

The transaction pool

@github-actions github-actions bot added A-p2p Related to P2P. A-dependency Related to dependencies, or changes to a Cargo.{toml,lock} file. A-workspace Changes to a root workspace file or general repo file. A-storage Related to storage. A-binaries Related to binaries. labels Oct 13, 2024
binaries/cuprated/src/txpool/incoming_tx.rs Show resolved Hide resolved
return;
};

// TODO: There is a race condition possible if a tx and block come in at the same time: <https://github.com/Cuprate/cuprate/issues/314>.
Copy link
Member Author

Choose a reason for hiding this comment

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

The REORG_LOCK is not enough to prevent certain situations, we will need a lock for every blockchain mutation. This shouldn't be too bad IMO, every ~2 mins needing to take a write lock, read locks will only need to be taken when multiple services are involved in handling a request,

Copy link
Member Author

Choose a reason for hiding this comment

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

to be clear all this would allow is a double spend tx in the pool, and this would only happen in a very unlucky situation.

So not really an issue however IMO we should aim to be correct under all possible scenarios, being lax here might have consequence elsewhere.

storage/txpool/src/free.rs Outdated Show resolved Hide resolved
storage/txpool/src/service/write.rs Show resolved Hide resolved
@Boog900 Boog900 marked this pull request as ready for review October 15, 2024 20:02
@Boog900 Boog900 requested a review from hinto-janai October 15, 2024 20:03
@Boog900
Copy link
Member Author

Boog900 commented Oct 15, 2024

there are some more things that need to be worked on for the txpool like: #316 #317

Copy link
Contributor

@hinto-janai hinto-janai left a comment

Choose a reason for hiding this comment

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

only nits, will review again

p2p/dandelion-tower/src/router.rs Outdated Show resolved Hide resolved
p2p/dandelion-tower/src/router.rs Outdated Show resolved Hide resolved
storage/txpool/src/ops/tx_read.rs Outdated Show resolved Hide resolved
storage/txpool/src/service/interface.rs Outdated Show resolved Hide resolved
storage/txpool/src/service/interface.rs Outdated Show resolved Hide resolved
storage/txpool/src/service/read.rs Outdated Show resolved Hide resolved
@hinto-janai
Copy link
Contributor

Btw the txpool issues should probably be C-discussion or have TODOs, C-proposal is for laid out problems + solutions.

@Boog900 Boog900 requested a review from hinto-janai October 22, 2024 23:53
@Boog900
Copy link
Member Author

Boog900 commented Oct 22, 2024

I'm planning to mark the issues as help wanted when this is merged - I'll remove the C-proposal

binaries/cuprated/src/blockchain/interface.rs Show resolved Hide resolved

/// An identifier for a P2P peer on any network.
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum CrossNetworkInternalPeerId {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this is defined here instead of in cuprate-p2p?

Copy link
Member Author

Choose a reason for hiding this comment

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

defining it here means we are more free to change it without it being a breaking change + this isn't used in cuprate-p2p.

binaries/cuprated/src/txpool.rs Outdated Show resolved Hide resolved
Comment on lines 21 to 22
/// TODO: should we expose this? probably not.
const DANDELION_CONFIG: DandelionConfig = DandelionConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is eventually exposed, it could/should be an associated constant on the type:

impl DandelionConfig {
    const CUPRATED_CONFIG: Self = /*...*/
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant exposed to users of cuprated, I'll update the comment.

binaries/cuprated/src/txpool/dandelion/diffuse_service.rs Outdated Show resolved Hide resolved
binaries/cuprated/src/txpool/incoming_tx.rs Outdated Show resolved Hide resolved
binaries/cuprated/src/txpool/incoming_tx.rs Outdated Show resolved Hide resolved
binaries/cuprated/src/txpool/incoming_tx.rs Outdated Show resolved Hide resolved
storage/txpool/src/service/interface.rs Outdated Show resolved Hide resolved
storage/txpool/src/service/write.rs Outdated Show resolved Hide resolved
@Boog900 Boog900 merged commit b57ee2f into main Oct 29, 2024
6 checks passed
@Boog900 Boog900 deleted the cuprated-txpool branch October 29, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-binaries Related to binaries. A-dependency Related to dependencies, or changes to a Cargo.{toml,lock} file. A-p2p Related to P2P. A-storage Related to storage. A-workspace Changes to a root workspace file or general repo file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants