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
Transactions gossiping #649
Changes from 1 commit
17df445
53fa71b
86420c6
e6cb8c9
a0ad196
b43e4dd
f93c3d5
99680f1
4c5a1fd
5c882e9
0ff8fc7
969b496
1fc5dac
94f27db
7c7a729
d539c6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,13 @@ use storage::{GenericStorage, ShardChainStorage, Trie, TrieUpdate}; | |
const POISONED_LOCK_ERR: &str = "The lock was poisoned."; | ||
|
||
pub mod pool_task; | ||
pub mod tx_gossip; | ||
|
||
use crate::pool_task::MemPoolControl; | ||
use primitives::signature::SecretKey; | ||
|
||
#[macro_use] | ||
extern crate serde_derive; | ||
|
||
/// mempool that stores transactions and receipts for a chain | ||
pub struct Pool { | ||
|
@@ -30,6 +35,7 @@ pub struct Pool { | |
pub authority_id: RwLock<Option<AuthorityId>>, | ||
/// Number of authorities currently. | ||
num_authorities: RwLock<Option<usize>>, | ||
owner_secret_key: RwLock<Option<SecretKey>>, | ||
/// Map from hash of tx/receipt to hashset of authorities it is known. | ||
known_to: RwLock<HashMap<CryptoHash, HashSet<AuthorityId>>>, | ||
/// List of requested snapshots that can't be fetched yet. | ||
|
@@ -49,6 +55,7 @@ impl Pool { | |
snapshots: Default::default(), | ||
authority_id: Default::default(), | ||
num_authorities: Default::default(), | ||
owner_secret_key: Default::default(), | ||
known_to: Default::default(), | ||
pending_snapshots: Default::default(), | ||
ready_snapshots: Default::default(), | ||
|
@@ -58,15 +65,17 @@ impl Pool { | |
/// Reset MemPool: clear snapshots, switch to new authorities and own authority id. | ||
pub fn reset(&self, control: MemPoolControl) { | ||
match control { | ||
MemPoolControl::Reset { authority_id, num_authorities, .. } => { | ||
MemPoolControl::Reset { authority_id, num_authorities, owner_secret_key, .. } => { | ||
info!(target: "mempool", "MemPool reset for {}", authority_id); | ||
*self.authority_id.write().expect(POISONED_LOCK_ERR) = Some(authority_id); | ||
*self.num_authorities.write().expect(POISONED_LOCK_ERR) = Some(num_authorities); | ||
*self.owner_secret_key.write().expect(POISONED_LOCK_ERR) = Some(owner_secret_key); | ||
} | ||
MemPoolControl::Stop => { | ||
info!(target: "mempool", "MemPool stopped"); | ||
*self.authority_id.write().expect(POISONED_LOCK_ERR) = None; | ||
*self.num_authorities.write().expect(POISONED_LOCK_ERR) = None; | ||
*self.owner_secret_key.write().expect(POISONED_LOCK_ERR) = None; | ||
} | ||
} | ||
self.snapshots.write().expect(POISONED_LOCK_ERR).clear(); | ||
|
@@ -105,6 +114,7 @@ impl Pool { | |
originator | ||
)); | ||
} | ||
self.known_to.write().expect(POISONED_LOCK_ERR).insert(transaction.get_hash(), HashSet::new()); | ||
self.transactions.write().expect(POISONED_LOCK_ERR).insert(transaction); | ||
Ok(()) | ||
} | ||
|
@@ -137,9 +147,27 @@ impl Pool { | |
Ok(()) | ||
} | ||
|
||
pub fn add_payload_with_author(&self, payload: ChainPayload, author: AuthorityId) -> Result<(), String> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this function has exactly the same signature (and almost the same logic) as function below - add_payload_snapshot There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. their the expectation is that it's final snapshot of proposal though, but seems reasonable to do the same things for keeping track of tx/receipts/known |
||
for transaction in payload.transactions { | ||
let hash = transaction.get_hash(); | ||
self.add_transaction(transaction)?; | ||
self.known_to | ||
.write() | ||
.expect(POISONED_LOCK_ERR) | ||
.entry(hash) | ||
.or_insert(HashSet::new()) | ||
.insert(author); | ||
} | ||
for receipt in payload.receipts { | ||
self.add_receipt(receipt)?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not tracking receipts as well? |
||
} | ||
Ok(()) | ||
} | ||
|
||
pub fn snapshot_payload(&self) -> CryptoHash { | ||
let transactions: Vec<_> = | ||
self.transactions.write().expect(POISONED_LOCK_ERR).drain().collect(); | ||
self.known_to.write().expect(POISONED_LOCK_ERR).drain(); | ||
let receipts: Vec<_> = self.receipts.write().expect(POISONED_LOCK_ERR).drain().collect(); | ||
let snapshot = ChainPayload { transactions, receipts }; | ||
if snapshot.is_empty() { | ||
|
@@ -188,6 +216,7 @@ impl Pool { | |
|
||
pub fn import_block(&self, block: &SignedShardBlock) { | ||
for transaction in block.body.transactions.iter() { | ||
self.known_to.write().expect(POISONED_LOCK_ERR).remove(&transaction.get_hash()); | ||
self.transactions.write().expect(POISONED_LOCK_ERR).remove(transaction); | ||
} | ||
for receipt in block.body.receipts.iter() { | ||
|
@@ -263,6 +292,7 @@ mod tests { | |
let transaction = SignedTransaction::new(signature, tx_body); | ||
pool.add_transaction(transaction.clone()).unwrap(); | ||
assert_eq!(pool.transactions.read().expect(POISONED_LOCK_ERR).len(), 1); | ||
assert_eq!(pool.known_to.read().expect(POISONED_LOCK_ERR).len(), 1); | ||
let block = SignedShardBlock::new( | ||
0, | ||
0, | ||
|
@@ -274,5 +304,6 @@ mod tests { | |
); | ||
pool.import_block(&block); | ||
assert_eq!(pool.transactions.read().expect(POISONED_LOCK_ERR).len(), 0); | ||
assert_eq!(pool.known_to.read().expect(POISONED_LOCK_ERR).len(), 0); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -18,6 +18,10 @@ use primitives::types::AuthorityId; | |||||
use nightshade::nightshade_task::Control; | ||||||
|
||||||
use crate::Pool; | ||||||
use crate::tx_gossip::TxGossip; | ||||||
use std::collections::HashSet; | ||||||
|
||||||
const POISONED_LOCK_ERR: &str = "The lock was poisoned."; | ||||||
|
||||||
#[derive(Clone, Debug)] | ||||||
pub enum MemPoolControl { | ||||||
|
@@ -43,7 +47,10 @@ pub fn spawn_pool( | |||||
payload_announce_tx: Sender<(AuthorityId, ChainPayload)>, | ||||||
payload_request_tx: Sender<PayloadRequest>, | ||||||
payload_response_rx: Receiver<PayloadResponse>, | ||||||
inc_tx_gossip_rx: Receiver<TxGossip>, | ||||||
out_tx_gossip_tx: Sender<TxGossip>, | ||||||
payload_announce_period: Duration, | ||||||
gossip_tx_period: Duration, | ||||||
) { | ||||||
// Handle request from NightshadeTask for confirmation on a payload. | ||||||
// If the payload can't be built from the mempool task to fetch necessary data is spawned and the | ||||||
|
@@ -176,4 +183,73 @@ pub fn spawn_pool( | |||||
future::ok(()) | ||||||
}); | ||||||
tokio::spawn(task); | ||||||
|
||||||
// Receive transaction gossips | ||||||
let pool5 = pool.clone(); | ||||||
let task = inc_tx_gossip_rx.for_each(move |tx_gossip| { | ||||||
// TODO: verify signature | ||||||
if let Err(e) = pool5.add_payload_with_author(tx_gossip.payload, tx_gossip.sender_id) { | ||||||
warn!(target: "pool", "Failed to add payload from tx gossip: {}", e); | ||||||
} | ||||||
|
||||||
future::ok(()) | ||||||
}); | ||||||
tokio::spawn(task); | ||||||
|
||||||
let pool6 = pool.clone(); | ||||||
let task = Interval::new_interval(gossip_tx_period) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the code above:
is suppose to do exactly the same thing. |
||||||
.for_each(move |_| { | ||||||
let my_authority_id = match *pool6.authority_id.read().expect(POISONED_LOCK_ERR) { | ||||||
Some(x) => { x }, | ||||||
None => { return future::ok(()) }, | ||||||
}; | ||||||
for their_authority_id in 0..pool6.num_authorities.read().expect(POISONED_LOCK_ERR).unwrap_or(0) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you probably should not send to all authorities all the time? the point of gossip is to send to subset from time to time IMO. |
||||||
if their_authority_id == my_authority_id { | ||||||
continue; | ||||||
} | ||||||
let sk = match pool6.owner_secret_key.write().expect(POISONED_LOCK_ERR).clone() { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should probably be outside the for loop. |
||||||
Some(sk) => sk, | ||||||
None => continue | ||||||
}; | ||||||
|
||||||
let mut to_send = vec![]; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all this logic should go into |
||||||
for tx in pool6.transactions.read().expect(POISONED_LOCK_ERR).iter() { | ||||||
let mut locked_known_to = pool6.known_to.write().expect(POISONED_LOCK_ERR); | ||||||
match locked_known_to.get_mut(&tx.get_hash()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||||||
Some(known_to) => { | ||||||
if !known_to.contains(&their_authority_id) { | ||||||
to_send.push(tx.clone()); | ||||||
known_to.insert(their_authority_id); | ||||||
} | ||||||
}, | ||||||
None => { | ||||||
to_send.push(tx.clone()); | ||||||
let mut known_to = HashSet::new(); | ||||||
known_to.insert(their_authority_id); | ||||||
locked_known_to.insert(tx.get_hash(), known_to); | ||||||
}, | ||||||
} | ||||||
} | ||||||
if to_send.len() == 0 { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
continue; | ||||||
} | ||||||
let payload = ChainPayload{ transactions: to_send, receipts: vec![] }; | ||||||
let gossip = TxGossip::new( | ||||||
my_authority_id, | ||||||
their_authority_id, | ||||||
payload, | ||||||
sk, | ||||||
); | ||||||
tokio::spawn( | ||||||
out_tx_gossip_tx | ||||||
.clone() | ||||||
.send(gossip) | ||||||
.map(|_| ()) | ||||||
.map_err(|e| warn!(target: "pool", "Error sending message: {}", e)), | ||||||
); | ||||||
} | ||||||
future::ok(()) | ||||||
}) | ||||||
.map_err(|e| error!(target: "pool", "Timer error: {}", e)); | ||||||
tokio::spawn(task); | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
use primitives::types::AuthorityId; | ||
use primitives::chain::ChainPayload; | ||
use primitives::signature::Signature; | ||
use primitives::hash::hash_struct; | ||
use primitives::signature::SecretKey; | ||
use primitives::signature::sign; | ||
|
||
#[derive(Debug, Serialize, Deserialize, Eq, PartialEq)] | ||
pub struct TxGossip { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be PayloadGossip? |
||
pub sender_id: AuthorityId, | ||
pub receiver_id: AuthorityId, | ||
pub payload: ChainPayload, | ||
signature: Signature, | ||
} | ||
|
||
impl TxGossip { | ||
pub fn new(sender_id: AuthorityId, receiver_id: AuthorityId, payload: ChainPayload, sk: SecretKey) -> Self { | ||
let hash = hash_struct(&(receiver_id, &payload)); | ||
TxGossip { | ||
sender_id, | ||
receiver_id, | ||
payload, | ||
signature: sign(hash.as_ref(), &sk), | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
use serde_derive::{Deserialize, Serialize}; | ||
|
||
use nightshade::nightshade_task::Gossip; | ||
use mempool::tx_gossip::TxGossip; | ||
use primitives::beacon::SignedBeaconBlock; | ||
use primitives::chain::{ChainPayload, ReceiptBlock, SignedShardBlock}; | ||
use primitives::hash::CryptoHash; | ||
|
@@ -45,6 +46,8 @@ pub enum Message { | |
|
||
/// Nightshade gossip. | ||
Gossip(Box<Gossip>), | ||
/// Transaction gossip | ||
TxGossip(Box<TxGossip>), | ||
/// Announce of tx/receipts between authorities. | ||
PayloadAnnounce(ChainPayload), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should remove payload announce then - that was suppose to be to send each other payloads. |
||
/// Request specific tx/receipts. | ||
|
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.
secret keys shouldn't be sent around. We have {Block/Transaction}Signer interface instead.
See #653 for the propagation of that.