Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Transaction Pool docs #9056

Merged
merged 8 commits into from Jun 23, 2021
Merged

Transaction Pool docs #9056

merged 8 commits into from Jun 23, 2021

Conversation

tomusdrw
Copy link
Contributor

@tomusdrw tomusdrw commented Jun 9, 2021

Additional high-level documentation for the transaciton pool. Includes a problem statement that should be independent from the current implementation and some implementation hints + high level description of the current code.

@tomusdrw tomusdrw added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Jun 9, 2021
@tomusdrw tomusdrw requested a review from NikVolf as a code owner June 9, 2021 15:59
Copy link
Contributor

@tomaka tomaka left a comment

Choose a reason for hiding this comment

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

Note that my suggestions about English languages are suggestions, I'm not good at language

client/transaction-pool/README.md Outdated Show resolved Hide resolved
Comment on lines +13 to +14
same (see implementation notes below).
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation notes don't mention anything about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an extra paragraph on that.

client/transaction-pool/README.md Outdated Show resolved Hide resolved
client/transaction-pool/README.md Outdated Show resolved Hide resolved
This parameter instructs the pool propagate/gossip a transaction to node peers.
By default this should be `true`, however in some cases it might be undesirable
to propagate transactions further (heavy transactions, privacy leaking, etc).
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the "privacy leaking" part. The transactions that are in a block's body are always equal to the one in the pool, no? Even if you're not propagating a transaction, once it's included in a block, the block itself will be propagated.

Copy link
Contributor Author

@tomusdrw tomusdrw Jun 9, 2021

Choose a reason for hiding this comment

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

I expanded the paragraph a bit cause it wasn't phrased very well. The point was mostly about being front runned by someone. Imagine that as a block author you find some solution to an on-chain puzzle and instead of propagating it you only include it in your own block. Or you want to report equivocation (a be rewarded for that) without letting anyone else to simply copy your finding.

client/transaction-pool/README.md Outdated Show resolved Hide resolved
The transaction pool is responsible for maintaining a set of transactions that
possible to include by block authors in upcoming blocks. Transactions are received
either from networking (gossiped by other peers) or RPC (submitted locally).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it should mention that when receiving transactions with the same bytes multiple times, it is only included once in the pool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is something that we deduct indirectly from provides tag description. I've added a point to "Implementation suggestions". Let me know if that suffices.

Copy link
Contributor

@tomaka tomaka Jun 10, 2021

Choose a reason for hiding this comment

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

Actually this is something that we deduct indirectly from provides tag description.

That's an interest take on this, but it implies that we should add the same transaction multiple times in the pool in case the provides tag is empty.

But this raises lots of questions, for example: if you receive the same transaction twice from the network, how do you know whether they're a unique transaction that's been gossiped around, or if it's actually two different people who have submitted the same transaction?
And if we prune transactions from the pool only if a transaction whose provides overlaps has been included in a block, that means we'd never prune transactions where provides is empty?

Copy link
Contributor Author

@tomusdrw tomusdrw Jun 10, 2021

Choose a reason for hiding this comment

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

That's an interest take on this, but it implies that we should add the same transaction multiple times in the pool in case the provides tag is empty.

The provides tag list cannot be empty (I mention that in the document) - such transactions should be considered invalid, we could panic in the runtime call actually if someone tries to do that, but the current pool implementation refuses to import such transaction for sure.

if it's actually two different people who have submitted the same transaction?

If the transaction is signed, it will contain two different signatures, so it's not the exact same payload. If it's unsigned, then well, I'd argue that de-duplication is sensible, cause it's literally nothing that allows us to distinguish one transaction from the other (or identify the sender for instance).

tomusdrw and others added 2 commits June 9, 2021 21:04
Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>
Fixed typos / spellings
@gilescope
Copy link
Contributor

Fantastic read - thank you so much for documenting this.

@tomusdrw tomusdrw merged commit 550d64c into master Jun 23, 2021
@tomusdrw tomusdrw deleted the td-transaction-pool-doc branch June 23, 2021 22:10
athei pushed a commit that referenced this pull request Jun 25, 2021
* Add transaction pool docs.

* Extra docs.

* Apply suggestions from code review

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>

* Expand on some review comments.

* Update README.md

Fixed typos / spellings

Co-authored-by: Pierre Krieger <pierre.krieger1708@gmail.com>
Co-authored-by: Squirrel <gilescope@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants