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

Deprecate AnnounceAccounts #10024

Merged
merged 7 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 2 additions & 23 deletions chain/client/src/adapter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ use near_primitives::block::{Approval, Block, BlockHeader};
use near_primitives::challenge::Challenge;
use near_primitives::errors::InvalidTxError;
use near_primitives::hash::CryptoHash;
use near_primitives::network::{AnnounceAccount, PeerId};
use near_primitives::network::PeerId;
use near_primitives::sharding::PartialEncodedChunk;
use near_primitives::transaction::SignedTransaction;
use near_primitives::types::{AccountId, EpochId, ShardId};
use near_primitives::types::{AccountId, ShardId};
use near_primitives::views::FinalExecutionOutcomeView;

/// Transaction status query
Expand Down Expand Up @@ -78,13 +78,6 @@ pub struct StateRequestPart {
#[rtype(result = "()")]
pub struct StateResponse(pub Box<StateResponseInfo>);

/// Account announcements that needs to be validated before being processed.
/// They are paired with last epoch id known to this announcement, in order to accept only
/// newer announcements.
#[derive(actix::Message, Debug)]
#[rtype(result = "Result<Vec<AnnounceAccount>,ReasonForBan>")]
pub(crate) struct AnnounceAccountRequest(pub Vec<(AnnounceAccount, Option<EpochId>)>);

#[derive(actix::Message, Debug)]
#[rtype(result = "()")]
pub struct SetNetworkInfo(pub NetworkInfo);
Expand Down Expand Up @@ -319,18 +312,4 @@ impl near_network::client::Client for Adapter {
Err(err) => tracing::error!("mailbox error: {err}"),
}
}

async fn announce_account(
&self,
accounts: Vec<(AnnounceAccount, Option<EpochId>)>,
) -> Result<Vec<AnnounceAccount>, ReasonForBan> {
match self.view_client_addr.send(AnnounceAccountRequest(accounts).with_span_context()).await
{
Ok(res) => res,
Err(err) => {
tracing::error!("mailbox error: {err}");
Ok(vec![])
}
}
}
}
67 changes: 2 additions & 65 deletions chain/client/src/client_actor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ use near_primitives::block::Tip;
use near_primitives::block_header::ApprovalType;
use near_primitives::epoch_manager::RngSeed;
use near_primitives::hash::CryptoHash;
use near_primitives::network::{AnnounceAccount, PeerId};
use near_primitives::network::PeerId;
use near_primitives::static_clock::StaticClock;
use near_primitives::types::{BlockHeight, ShardId};
use near_primitives::unwrap_or_return;
Expand All @@ -74,7 +74,7 @@ use std::collections::HashMap;
use std::fmt;
use std::sync::{Arc, RwLock};
use std::thread;
use std::time::{Duration, Instant};
use std::time::Duration;
use tokio::sync::broadcast;
use tracing::{debug, error, info, trace, warn};

Expand All @@ -96,8 +96,6 @@ pub struct ClientActor {
/// Identity that represents this Client at the network level.
/// It is used as part of the messages that identify this client.
node_id: PeerId,
/// Last time we announced our accounts as validators.
last_validator_announce_time: Option<Instant>,
/// Info helper.
info_helper: InfoHelper,

Expand Down Expand Up @@ -195,7 +193,6 @@ impl ClientActor {
tier1_accounts_keys: vec![],
tier1_accounts_data: vec![],
},
last_validator_announce_time: None,
info_helper,
block_production_next_attempt: now,
log_summary_timer_next_attempt: now,
Expand Down Expand Up @@ -855,60 +852,6 @@ impl fmt::Display for SyncRequirement {
}

impl ClientActor {
/// Check if client Account Id should be sent and send it.
/// Account Id is sent when is not current a validator but are becoming a validator soon.
fn check_send_announce_account(&mut self, prev_block_hash: CryptoHash) {
// If no peers, there is no one to announce to.
if self.network_info.num_connected_peers == 0 {
debug!(target: "client", "No peers: skip account announce");
return;
}

// First check that we currently have an AccountId
let validator_signer = match self.client.validator_signer.as_ref() {
None => return,
Some(signer) => signer,
};

let now = StaticClock::instant();
// Check that we haven't announced it too recently
if let Some(last_validator_announce_time) = self.last_validator_announce_time {
// Don't make announcement if have passed less than half of the time in which other peers
// should remove our Account Id from their Routing Tables.
if 2 * (now - last_validator_announce_time) < self.client.config.ttl_account_id_router {
return;
}
}

debug!(target: "client", "Check announce account for {}, last announce time {:?}", validator_signer.validator_id(), self.last_validator_announce_time);

// Announce AccountId if client is becoming a validator soon.
let next_epoch_id = unwrap_or_return!(self
.client
.epoch_manager
.get_next_epoch_id_from_prev_block(&prev_block_hash));

// Check client is part of the futures validators
if self.client.is_validator(&next_epoch_id, &prev_block_hash) {
debug!(target: "client", "Sending announce account for {}", validator_signer.validator_id());
self.last_validator_announce_time = Some(now);

let signature = validator_signer.sign_account_announce(
validator_signer.validator_id(),
&self.node_id,
&next_epoch_id,
);
self.network_adapter.send(PeerManagerMessageRequest::NetworkRequests(
NetworkRequests::AnnounceAccount(AnnounceAccount {
account_id: validator_signer.validator_id().clone(),
peer_id: self.node_id.clone(),
epoch_id: next_epoch_id,
signature,
}),
));
}
}

/// Process the sandbox fast forward request. If the change in block height is past an epoch,
/// we fast forward to just right before the epoch, produce some blocks to get past and into
/// a new epoch, then we continue on with the residual amount to fast forward.
Expand Down Expand Up @@ -1348,7 +1291,6 @@ impl ClientActor {
let block = self.client.chain.get_block(&accepted_block).unwrap().clone();
self.send_chunks_metrics(&block);
self.send_block_metrics(&block);
self.check_send_announce_account(*block.header().last_final_block());
}
}

Expand Down Expand Up @@ -1582,11 +1524,6 @@ impl ClientActor {
if currently_syncing {
info!(target: "client", "disabling sync: {}", &sync);
self.client.sync_status = SyncStatus::NoSync;

// Initial transition out of "syncing" state.
// Announce this client's account id if their epoch is coming up.
let head = unwrap_and_report!(self.client.chain.head());
self.check_send_announce_account(head.prev_block_hash);
}
}

Expand Down
27 changes: 3 additions & 24 deletions chain/client/src/test_utils/setup.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
use super::block_stats::BlockStats;
use super::peer_manager_mock::PeerManagerMock;
use crate::adapter::{
AnnounceAccountRequest, BlockApproval, BlockHeadersRequest, BlockHeadersResponse, BlockRequest,
BlockResponse, SetNetworkInfo, StateRequestHeader, StateRequestPart,
BlockApproval, BlockHeadersRequest, BlockHeadersResponse, BlockRequest, BlockResponse,
SetNetworkInfo, StateRequestHeader, StateRequestPart,
};
use crate::{start_view_client, Client, ClientActor, SyncAdapter, SyncStatus, ViewClientActor};
use actix::{Actor, Addr, AsyncContext, Context};
Expand Down Expand Up @@ -54,7 +54,7 @@ use num_rational::Ratio;
use once_cell::sync::OnceCell;
use rand::{thread_rng, Rng};
use std::cmp::max;
use std::collections::{HashMap, HashSet};
use std::collections::HashMap;
use std::ops::DerefMut;
use std::sync::{Arc, RwLock};
use std::time::{Duration, Instant};
Expand Down Expand Up @@ -450,7 +450,6 @@ pub fn setup_mock_all_validators(

let connectors: Arc<OnceCell<Vec<ActorHandlesForTesting>>> = Default::default();

let announced_accounts = Arc::new(RwLock::new(HashSet::new()));
let genesis_block = Arc::new(RwLock::new(None));

let last_height = Arc::new(RwLock::new(vec![0; key_pairs.len()]));
Expand All @@ -471,7 +470,6 @@ pub fn setup_mock_all_validators(
let addresses = addresses.clone();
let connectors1 = connectors.clone();
let network_mock1 = peer_manager_mock.clone();
let announced_accounts1 = announced_accounts.clone();
let last_height1 = last_height.clone();
let last_height2 = last_height.clone();
let largest_endorsed_height1 = largest_endorsed_height.clone();
Expand Down Expand Up @@ -742,25 +740,6 @@ pub fn setup_mock_all_validators(
);
}
}
NetworkRequests::AnnounceAccount(announce_account) => {
let mut aa = announced_accounts1.write().unwrap();
let key = (
announce_account.account_id.clone(),
announce_account.epoch_id.clone(),
);
if aa.get(&key).is_none() {
aa.insert(key);
for actor_handles in connectors1 {
actor_handles.view_client_actor.do_send(
AnnounceAccountRequest(vec![(
announce_account.clone(),
None,
)])
.with_span_context(),
)
}
}
}
NetworkRequests::Approval { approval_message } => {
let height_mod = approval_message.approval.target_height % 300;

Expand Down
87 changes: 4 additions & 83 deletions chain/client/src/view_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
//! Useful for querying from RPC.

use crate::adapter::{
AnnounceAccountRequest, BlockHeadersRequest, BlockRequest, StateRequestHeader,
StateRequestPart, StateResponse, TxStatusRequest, TxStatusResponse,
BlockHeadersRequest, BlockRequest, StateRequestHeader, StateRequestPart, StateResponse,
TxStatusRequest, TxStatusResponse,
};
use crate::{
metrics, sync, GetChunk, GetExecutionOutcomeResponse, GetNextLightClientBlock, GetStateChanges,
Expand All @@ -30,16 +30,15 @@ use near_client_primitives::types::{
use near_epoch_manager::shard_tracker::ShardTracker;
use near_epoch_manager::EpochManagerAdapter;
use near_network::types::{
NetworkRequests, PeerManagerAdapter, PeerManagerMessageRequest, ReasonForBan,
StateResponseInfo, StateResponseInfoV2,
NetworkRequests, PeerManagerAdapter, PeerManagerMessageRequest, StateResponseInfo,
StateResponseInfoV2,
};
use near_o11y::{handler_debug_span, OpenTelemetrySpanExt, WithSpanContext, WithSpanContextExt};
use near_performance_metrics_macros::perf;
use near_primitives::block::{Block, BlockHeader};
use near_primitives::epoch_manager::epoch_info::EpochInfo;
use near_primitives::hash::CryptoHash;
use near_primitives::merkle::{merklize, PartialMerkleTree};
use near_primitives::network::AnnounceAccount;
use near_primitives::receipt::Receipt;
use near_primitives::sharding::ShardChunk;
use near_primitives::state_sync::{
Expand All @@ -59,7 +58,6 @@ use near_primitives::views::{
};
use near_store::flat::{FlatStorageReadyStatus, FlatStorageStatus};
use near_store::{DBCol, COLD_HEAD_KEY, FINAL_HEAD_KEY, HEAD_KEY};
use std::cmp::Ordering;
use std::collections::{BTreeSet, HashMap, VecDeque};
use std::hash::Hash;
use std::sync::{Arc, Mutex, RwLock};
Expand Down Expand Up @@ -542,24 +540,6 @@ impl ViewClientActor {
self.chain.retrieve_headers(hashes, sync::header::MAX_BLOCK_HEADERS, None)
}

fn check_signature_account_announce(
&self,
announce_account: &AnnounceAccount,
) -> Result<bool, Error> {
let announce_hash = announce_account.hash();
let head = self.chain.head()?;

self.epoch_manager
.verify_validator_signature(
&announce_account.epoch_id,
&head.last_block_hash,
&announce_account.account_id,
announce_hash.as_ref(),
&announce_account.signature,
)
.map_err(|e| e.into())
}

/// Returns true if this request needs to be **dropped** due to exceeding a
/// rate limit of state sync requests.
fn throttle_state_sync_request(&self) -> bool {
Expand Down Expand Up @@ -1460,65 +1440,6 @@ impl Handler<WithSpanContext<StateRequestPart>> for ViewClientActor {
}
}

impl Handler<WithSpanContext<AnnounceAccountRequest>> for ViewClientActor {
type Result = Result<Vec<AnnounceAccount>, ReasonForBan>;

#[perf]
fn handle(
&mut self,
msg: WithSpanContext<AnnounceAccountRequest>,
_ctx: &mut Self::Context,
) -> Self::Result {
let (_span, msg) = handler_debug_span!(target: "client", msg);
tracing::debug!(target: "client", ?msg);
let _timer = metrics::VIEW_CLIENT_MESSAGE_TIME
.with_label_values(&["AnnounceAccountRequest"])
.start_timer();
let AnnounceAccountRequest(announce_accounts) = msg;

let mut filtered_announce_accounts = Vec::new();

for (announce_account, last_epoch) in announce_accounts {
// Keep the announcement if it is newer than the last announcement from
// the same account.
if let Some(last_epoch) = last_epoch {
match self.epoch_manager.compare_epoch_id(&announce_account.epoch_id, &last_epoch) {
Ok(Ordering::Greater) => {}
_ => continue,
}
}

match self.check_signature_account_announce(&announce_account) {
Ok(true) => {
filtered_announce_accounts.push(announce_account);
}
// TODO(gprusak): Here we ban for broadcasting accounts which have been slashed
// according to BlockInfo for the current chain tip. It is unfair,
// given that peers do not have perfectly synchronized heads:
// - AFAIU each block can introduce a slashed account, so the announcement
// could be OK at the moment that peer has sent it out.
// - the current epoch_id is not related to announce_account.epoch_id,
// so it carry a perfectly valid (outdated) information.
Ok(false) => {
return Err(ReasonForBan::InvalidSignature);
}
// Filter out this account. This covers both good reasons to ban the peer:
// - signature didn't match the data and public_key.
// - account is not a validator for the given epoch
// and cases when we were just unable to validate the data (so we shouldn't
// ban), for example when the node is not aware of the public key for the given
// (account_id,epoch_id) pair.
// We currently do NOT ban the peer for either.
// TODO(gprusak): consider whether we should change that.
Err(err) => {
tracing::debug!(target: "view_client", ?err, "Failed to validate account announce signature");
}
}
}
Ok(filtered_announce_accounts)
}
}

impl Handler<WithSpanContext<GetGasPrice>> for ViewClientActor {
type Result = Result<GasPriceView, GetGasPriceError>;

Expand Down
2 changes: 1 addition & 1 deletion chain/jsonrpc/res/last_blocks.js
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ function Page() {
<div className="explanation">Skipped chunks have grey background.</div>
<div className="explanation">
Red text means that we don't know this producer
(it's not present in our announce account list).
(we haven't received the account data).
</div>
{error && <div className="error">{error.stack}</div>}
<div className="missed-blocks">
Expand Down
5 changes: 2 additions & 3 deletions chain/jsonrpc/res/network_info.html
Original file line number Diff line number Diff line change
Expand Up @@ -258,9 +258,8 @@ <h2>
Unreachable: <span class="js-unreachable-chunk-producers"></span>
</pre>

<b>Unknown</b> means that we didn't receive 'announce' information about this validator (so we don't know on which
peer it
is). This usually means that the validator didn't connect to the network
<b>Unknown</b> means that we didn't receive account data for this validator (so we don't know on which
peer it is). This usually means that the validator didn't connect to the network
during current epoch.

<br>
Expand Down
Loading
Loading