Changeset View
Changeset View
Standalone View
Standalone View
src/avalanche/processor.cpp
// Copyright (c) 2018-2019 The Bitcoin developers | // Copyright (c) 2018-2019 The Bitcoin developers | ||||
// Distributed under the MIT software license, see the accompanying | // Distributed under the MIT software license, see the accompanying | ||||
// file COPYING or http://www.opensource.org/licenses/mit-license.php. | // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||||
#include <avalanche/processor.h> | #include <avalanche/processor.h> | ||||
#include <avalanche/delegationbuilder.h> | #include <avalanche/delegationbuilder.h> | ||||
#include <avalanche/peermanager.h> | #include <avalanche/peermanager.h> | ||||
#include <avalanche/validation.h> | #include <avalanche/validation.h> | ||||
#include <chain.h> | #include <chain.h> | ||||
#include <key_io.h> // For DecodeSecret | #include <key_io.h> // For DecodeSecret | ||||
#include <net_processing.h> // For ::PeerManager | #include <net_processing.h> // For ::PeerManager | ||||
#include <netmessagemaker.h> | #include <netmessagemaker.h> | ||||
#include <reverse_iterator.h> | #include <reverse_iterator.h> | ||||
#include <scheduler.h> | #include <scheduler.h> | ||||
#include <util/bitmanip.h> | #include <util/bitmanip.h> | ||||
#include <util/translation.h> | |||||
#include <validation.h> | #include <validation.h> | ||||
#include <chrono> | #include <chrono> | ||||
#include <tuple> | #include <tuple> | ||||
/** | /** | ||||
* Run the avalanche event loop every 10ms. | * Run the avalanche event loop every 10ms. | ||||
*/ | */ | ||||
▲ Show 20 Lines • Show All 106 Lines • ▼ Show 20 Lines | static bool IsWorthPolling(const CBlockIndex *pindex) { | ||||
if (::ChainstateActive().IsBlockFinalized(pindex)) { | if (::ChainstateActive().IsBlockFinalized(pindex)) { | ||||
// There is no point polling finalized block. | // There is no point polling finalized block. | ||||
return false; | return false; | ||||
} | } | ||||
return true; | return true; | ||||
} | } | ||||
Processor::PeerData Processor::PeerData::FromArgs(const ArgsManager &argsman) { | bool PeerData::FromArgs(PeerData &peerData, const ArgsManager &argsman, | ||||
deadalnix: Ok, this is getting somewhere, but see, if you kept the smart pointer, you'd have been able to… | |||||
PeerData peerData; | bilingual_str &error) { | ||||
if (argsman.IsArgSet("-avasessionkey")) { | if (argsman.IsArgSet("-avasessionkey")) { | ||||
peerData.sessionKey = | peerData.sessionKey = | ||||
DecodeSecret(argsman.GetArg("-avasessionkey", "")); | DecodeSecret(argsman.GetArg("-avasessionkey", "")); | ||||
} else { | } else { | ||||
// Pick a random key for the session. | // Pick a random key for the session. | ||||
peerData.sessionKey.MakeNewKey(true); | peerData.sessionKey.MakeNewKey(true); | ||||
} | } | ||||
if (argsman.IsArgSet("-avaproof")) { | if (argsman.IsArgSet("-avaproof")) { | ||||
{ | { | ||||
// The proof. | // The proof. | ||||
CDataStream stream(ParseHex(argsman.GetArg("-avaproof", "")), | CDataStream stream(ParseHex(argsman.GetArg("-avaproof", "")), | ||||
SER_NETWORK, 0); | SER_NETWORK, 0); | ||||
Proof proof; | Proof proof; | ||||
stream >> proof; | stream >> proof; | ||||
// If a proof is supplied, make sure it does not contain garbage. | |||||
// At this point the validity of the utxos cannot be checked, so | |||||
// only basic verification is performed. | |||||
avalanche::ProofValidationState proof_state; | |||||
if (!proof.verify(proof_state)) { | |||||
switch (proof_state.GetResult()) { | |||||
case avalanche::ProofValidationResult::NO_STAKE: | |||||
error = _("the avalanche proof has no stake"); | |||||
return false; | |||||
case avalanche::ProofValidationResult::DUST_THRESOLD: | |||||
error = _("the avalanche proof stake is too low"); | |||||
return false; | |||||
case avalanche::ProofValidationResult::DUPLICATE_STAKE: | |||||
error = _("the avalanche proof has duplicated stake"); | |||||
return false; | |||||
case avalanche::ProofValidationResult::INVALID_SIGNATURE: | |||||
error = _("the avalanche proof has invalid stake " | |||||
"signatures"); | |||||
return false; | |||||
case avalanche::ProofValidationResult::TOO_MANY_UTXOS: | |||||
error = strprintf(_("the avalanche proof has too " | |||||
"many utxos (max: %u)"), | |||||
AVALANCHE_MAX_PROOF_STAKES); | |||||
return false; | |||||
default: | |||||
error = _("the avalanche proof is invalid"); | |||||
return false; | |||||
} | |||||
} | |||||
peerData.proof = proof; | peerData.proof = proof; | ||||
} | } | ||||
// Generate the delegation to the session key. | // Generate the delegation to the session key. | ||||
DelegationBuilder dgb(peerData.proof.value()); | DelegationBuilder dgb(peerData.proof.value()); | ||||
if (peerData.sessionKey.GetPubKey() != peerData.proof->getMaster()) { | if (peerData.sessionKey.GetPubKey() != peerData.proof->getMaster()) { | ||||
dgb.addLevel(DecodeSecret(argsman.GetArg("-avamasterkey", "")), | dgb.addLevel(DecodeSecret(argsman.GetArg("-avamasterkey", "")), | ||||
peerData.sessionKey.GetPubKey()); | peerData.sessionKey.GetPubKey()); | ||||
} | } | ||||
peerData.delegation = dgb.build(); | peerData.delegation = dgb.build(); | ||||
} | } | ||||
return peerData; | return true; | ||||
} | } | ||||
class Processor::NotificationsHandler | class Processor::NotificationsHandler | ||||
: public interfaces::Chain::Notifications { | : public interfaces::Chain::Notifications { | ||||
Processor *m_processor; | Processor *m_processor; | ||||
public: | public: | ||||
NotificationsHandler(Processor *p) : m_processor(p) {} | NotificationsHandler(Processor *p) : m_processor(p) {} | ||||
Show All 19 Lines | : connman(connmanIn), nodePeerManager(nodePeerManagerIn), | ||||
peerManager(std::make_unique<PeerManager>()) { | peerManager(std::make_unique<PeerManager>()) { | ||||
// Make sure we get notified of chain state changes. | // Make sure we get notified of chain state changes. | ||||
chainNotificationsHandler = | chainNotificationsHandler = | ||||
chain.handleNotifications(std::make_shared<NotificationsHandler>(this)); | chain.handleNotifications(std::make_shared<NotificationsHandler>(this)); | ||||
} | } | ||||
Processor::Processor(interfaces::Chain &chain, CConnman *connmanIn, | Processor::Processor(interfaces::Chain &chain, CConnman *connmanIn, | ||||
NodePeerManager *nodePeerManagerIn, | NodePeerManager *nodePeerManagerIn, | ||||
const ArgsManager &argsman) | const PeerData &peerDataIn) | ||||
: Processor(chain, connmanIn, nodePeerManagerIn) { | : Processor(chain, connmanIn, nodePeerManagerIn) { | ||||
peerData = PeerData::FromArgs(argsman); | peerData = peerDataIn; | ||||
deadalnixUnsubmitted Not Done Inline ActionsCan't you do a ctor style init? deadalnix: Can't you do a ctor style init? | |||||
if (peerData.proof) { | if (peerData.proof) { | ||||
// Schedule proof registration at the first new block after IBD. | // Schedule proof registration at the first new block after IBD. | ||||
mustRegisterProof = true; | mustRegisterProof = true; | ||||
deadalnixUnsubmitted Not Done Inline ActionsNot sure where this was introduced, but urg. This is pretty much the definition of temporal coupling: https://blog.ploeh.dk/2011/05/24/DesignSmellTemporalCoupling/ deadalnix: Not sure where this was introduced, but urg. This is pretty much the definition of temporal… | |||||
} | } | ||||
} | } | ||||
Processor::~Processor() { | Processor::~Processor() { | ||||
chainNotificationsHandler.reset(); | chainNotificationsHandler.reset(); | ||||
stopEventLoop(); | stopEventLoop(); | ||||
} | } | ||||
▲ Show 20 Lines • Show All 420 Lines • Show Last 20 Lines |
Ok, this is getting somewhere, but see, if you kept the smart pointer, you'd have been able to return something nullable. Now you *HAVE TO* construct the PeerData object, which makes no sense when there is an error.
You threw away the guarantee that the PeerData object is properly constructed with that design.
But then you'll have another problem: what if this should actually be null, and it's not an error? Well, this is why the factory method should be about the processor, not the PeerData. Bonus point, you don't need to make PeerData part of the API.
It's all win, but you got to ask yourself the right question before going down some path. It's not some hypothetical future change that might fubar things up, this is the code you just wrote telling you this. Read it.