-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Transactions dependency graph #787
Conversation
core/transaction-graph/src/future.rs
Outdated
} | ||
|
||
impl<Hash> WaitingTransaction<Hash> { | ||
pub fn new(transaction: Transaction<Hash>, provided: &HashMap<Tag, Hash>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs
core/transaction-graph/src/future.rs
Outdated
} | ||
} | ||
|
||
pub fn satisfy_tag(&mut self, tag: &Tag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs
core/transaction-graph/src/future.rs
Outdated
self.missing_tags.remove(tag); | ||
} | ||
|
||
pub fn is_ready(&self) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs
core/transaction-graph/src/future.rs
Outdated
|
||
#[derive(Debug)] | ||
pub struct WaitingTransaction<Hash> { | ||
pub transaction: Transaction<Hash>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs
core/transaction-graph/src/future.rs
Outdated
use pool::Transaction; | ||
|
||
#[derive(Debug)] | ||
pub struct WaitingTransaction<Hash> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs
core/transaction-graph/src/future.rs
Outdated
#[derive(Debug)] | ||
pub struct WaitingTransaction<Hash> { | ||
pub transaction: Transaction<Hash>, | ||
pub missing_tags: HashSet<Tag>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs
core/transaction-graph/src/future.rs
Outdated
} | ||
|
||
#[derive(Debug)] | ||
pub struct FutureTransactions<Hash: hash::Hash + Eq> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs
core/transaction-graph/src/future.rs
Outdated
for hash in hashes { | ||
let is_ready = { | ||
let mut tx = self.waiting.get_mut(&hash) | ||
.expect("Every transaction in wanted_tags is present in waiting; qed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not strict enough. either prove how it is that they are present or manage the case where it's not gracefully. i think it's enough to just enclose in an if let
with the else
giving a warn!
and continue
so that failure is harmless and is reported.
core/transaction-graph/src/ready.rs
Outdated
pub requires_offset: usize, | ||
} | ||
|
||
const HASH_READY: &str = "Every hash is in ready map; qed"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more detail
|
||
/// Removes transactions that provide given tag. | ||
/// | ||
/// All transactions that lead to a transaction, which provides this tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
under what circumstances is this used?
i can't see why this would be used according to its spec.
if you have a tag graph:
A -> B -> C
D -> E ---^
if A, B and C get finalised and you use this to remove anything that provides tag C then you'll end up removing D and E, even though they are valid in and of themselves and could still be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if C
can now be pruned it means that either:
- All requirements for
C
are now satisfied, meaning bothA->B
andD->E
chains (or some replacement chain) had to be included as well. - Some other transaction provided
C
and it got finalized, which doesn't really tell us much about what happened to the others.
Indeed removing the transactions eagerly can be problematic (I've outlined one use case in the docs) - but the idea is that caller of that function may re-verify the transactions that got pruned and reimport them to the pool if they are still valid.
We prune the transactions eagerly for two reasons:
- If we possibly miss a block notification we will get the pool into consistent state
- If we don't do that we can easily end up with plenty stale transactions in the pool that will never really be included.
So since the pool cannot re-verify the transactions itself, I chose to remove potentially more than needed, but allowing the caller to reimport transactions if he figures they are still valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- not sure why this would be the case.
- i don't see why we would end up with stale transactions - if they can be included (and by definition, D and E still can, even when C has been included) then they should be in the queue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block notifications are spuriously missed, not delivered during "major sync" (can restart at any time if we go offline for a little, and are generally unreliable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If C
was included (and it required B
and D
) you cannot really include D
or D'
providing the same tag, as the assumption is that every tag can be provided by only one transaction.
Also as mentioned, this code will be wrapped in a higher-level struct that will attempt to revalidate all transactions to make sure if they are still OK and import them back if that's the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good aside from remarks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
core/transaction-graph/src/pool.rs
Outdated
/// NOTE some transactions might still be valid, but were just removed because | ||
/// they were part of a chain, you may attempt to re-import them later. | ||
/// NOTE If you want to just mark ready transactions that were already used | ||
/// and you no longer need to store them use `mark_used` method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is mark_used
method? Can't find it anywhere...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks! I meant prune_tags
method, and now rewrote the comment to reflect that.
core/transaction-graph/src/pool.rs
Outdated
/// they were part of a chain, you may attempt to re-import them later. | ||
/// NOTE If you want to just mark ready transactions that were already used | ||
/// and you no longer need to store them use `mark_used` method. | ||
pub fn remove_invalid(&mut self, hashes: &[Hash]) -> Vec<Arc<Transaction<Hash>>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this public? Why the external code might want to call this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For instance if we detect that the transaction causes panic when included in the block.
So the tags it provide won't get finalized, but we still need to remove it from the pool, so that it's not returned in subsequent calls to ready()
.
We also remove all dependent transactions obviously, the caller may choose to reverify and reimport them so that they will end up in the future part of the pool.
…ritytech#787) * version info with built-time obtained git hash * clippy * rerun-if-changed properly and handle git command failing * cargo fmt
A pool of transactions based only on dependencies between them and priorities.
First part of #728
PR introduces new crate, that is not yet connected to the rest of the code. The idea is that new transactions can be imported to the pool after we call
validate_transaction
in runtime and getTransactionValidity
(introduced in #741 )Every time a block is imported we are going to call
prune_tags
providing all tags that has been satisfied by transactions in that block. The method should work correctly even if we didn't know about some of the transactions, or we didn't receiveprune_tags
for some earlier blocks.Stuff that's still left: