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

Transactions gossiping #649

Merged
merged 16 commits into from Mar 7, 2019

Conversation

Projects
None yet
4 participants
@SkidanovAlex
Copy link
Member

SkidanovAlex commented Mar 2, 2019

No description provided.

@nearmax

nearmax approved these changes Mar 2, 2019

Copy link
Collaborator

nearmax left a comment

Would be good to have some tests that check that this feature works.

if their_authority_id == my_authority_id {
continue;
}
let sk = match pool6.owner_secret_key.write().expect(POISONED_LOCK_ERR).clone() {

This comment has been minimized.

@nearmax

nearmax Mar 2, 2019

Collaborator

This should probably be outside the for loop.

},
}
}
if to_send.len() == 0 {

This comment has been minimized.

@nearmax

nearmax Mar 2, 2019

Collaborator
Suggested change
if to_send.len() == 0 {
if to_send.is_empty() {
let mut to_send = vec![];
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()) {

This comment has been minimized.

@nearmax

nearmax Mar 2, 2019

Collaborator

Use locked_known_to.entry() instead, e.g. for the reference:

match penalties.entry(acc.account_id.clone()) {


use crate::pool_task::MemPoolControl;
use primitives::signature::SecretKey;

This comment has been minimized.

@ilblackdragon

ilblackdragon Mar 2, 2019

Member

secret keys shouldn't be sent around. We have {Block/Transaction}Signer interface instead.

See #653 for the propagation of that.

use primitives::signature::sign;

#[derive(Debug, Serialize, Deserialize, Eq, PartialEq)]
pub struct TxGossip {

This comment has been minimized.

@ilblackdragon

ilblackdragon Mar 2, 2019

Member

Should this be PayloadGossip?
Also should gossip first send Hashs of tx/receipts and then other client retrieves content if doesn't have it?

@@ -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),

This comment has been minimized.

@ilblackdragon

ilblackdragon Mar 2, 2019

Member

Should remove payload announce then - that was suppose to be to send each other payloads.

.insert(author);
}
for receipt in payload.receipts {
self.add_receipt(receipt)?;

This comment has been minimized.

@ilblackdragon

ilblackdragon Mar 2, 2019

Member

why not tracking receipts as well?

@@ -137,9 +147,27 @@ impl Pool {
Ok(())
}

pub fn add_payload_with_author(&self, payload: ChainPayload, author: AuthorityId) -> Result<(), String> {

This comment has been minimized.

@ilblackdragon

ilblackdragon Mar 2, 2019

Member

this function has exactly the same signature (and almost the same logic) as function below - add_payload_snapshot

This comment has been minimized.

@ilblackdragon

ilblackdragon Mar 2, 2019

Member

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

tokio::spawn(task);

let pool6 = pool.clone();
let task = Interval::new_interval(gossip_tx_period)

This comment has been minimized.

@ilblackdragon

ilblackdragon Mar 2, 2019

Member

the code above:

    let pool2 = pool.clone();
    let task = Interval::new_interval(payload_announce_period)
        .for_each(move |_| {
            if let Some((authority_id, payload)) = pool2.prepare_payload_announce() {

is suppose to do exactly the same thing.
the idea ws to put logic of deciding who to announce/gossip what payloads in prepare_payload_announce

None => continue
};

let mut to_send = vec![];

This comment has been minimized.

@ilblackdragon

ilblackdragon Mar 2, 2019

Member

all this logic should go into prepare_payload_announce.

Some(x) => { x },
None => { return future::ok(()) },
};
for their_authority_id in 0..pool6.num_authorities.read().expect(POISONED_LOCK_ERR).unwrap_or(0) {

This comment has been minimized.

@ilblackdragon

ilblackdragon Mar 2, 2019

Member

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.

@ilblackdragon

This comment has been minimized.

Copy link
Member

ilblackdragon commented Mar 2, 2019

I merge master in, just to simplify life.

@ilblackdragon

This comment has been minimized.

Copy link
Member

ilblackdragon commented Mar 2, 2019

Addressed some of my comments (=

@ilblackdragon

This comment has been minimized.

Copy link
Member

ilblackdragon commented Mar 2, 2019

Btw, in test_three_nodes_sync switching

        alice.client
            .shard_client
            .pool
            .add_transaction(
                TransactionBody::send_money(1, "alice.near", "bob.near", 10)
                    .sign(alice.signer()),
            )
            .unwrap();

to bob.client - doesn't work.
So gossip doesn't actually operate.

@ilblackdragon

This comment has been minimized.

Copy link
Member

ilblackdragon commented Mar 2, 2019

We need to change snapshots to be just hashes of tx/receipts. Right now they actually removing tx, which prevents them from gossiped / and when another snapshot is accepted - must not remove the tx that didn't end up there.

@nearmax

This comment has been minimized.

Copy link
Collaborator

nearmax commented Mar 5, 2019

We need to change snapshots to be just hashes of tx/receipts. Right now they actually removing tx, which prevents them from gossiped / and when another snapshot is accepted - must not remove the tx that didn't end up there.

Duplicate? #660

mfornet and others added some commits Mar 5, 2019

ilblackdragon added some commits Mar 7, 2019

Fixing bug is passthrough consensus: it was calling snapshot more oft…
…en then consensus was produced. Added devnet output for pynear tests

@ilblackdragon ilblackdragon merged commit 2995718 into master Mar 7, 2019

1 check passed

ci/gitlab/tx_gossip Pipeline passed on GitLab
Details

@ilblackdragon ilblackdragon deleted the tx_gossip branch Mar 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.