Skip to content

wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload #17484

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

wallet: add cached m_is_ibd to remove Chain::isInitialBlockDownload #17484

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
693bbc6
[interfaces] Extend Chain::Notifications::updatedBlockTip with bool i…
Nov 14, 2019
797ec9d
[wallet] Add m_is_ibd in CWallet
Nov 14, 2019
20eb0da
[interfaces] Remove Chain::isReadyToBroadcast
Nov 14, 2019
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
5 changes: 2 additions & 3 deletions src/interfaces/chain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class NotificationsProxy : public CValidationInterface
}
void UpdatedBlockTip(const CBlockIndex* index, const CBlockIndex* fork_index, bool is_ibd) override
{
m_notifications->updatedBlockTip();
m_notifications->updatedBlockTip(is_ibd);
}
void ChainStateFlushed(const CBlockLocator& locator) override { m_notifications->chainStateFlushed(locator); }
std::shared_ptr<Chain::Notifications> m_notifications;
Expand Down Expand Up @@ -339,8 +339,7 @@ class ChainImpl : public Chain
LOCK(cs_main);
return ::fHavePruned;
}
bool isReadyToBroadcast() override { return !::fImporting && !::fReindex && !isInitialBlockDownload(); }
bool isInitialBlockDownload() override { return ::ChainstateActive().IsInitialBlockDownload(); }
bool isReadyToBroadcast() override { return !::fImporting && !::fReindex; }
bool shutdownRequested() override { return ShutdownRequested(); }
int64_t getAdjustedTime() override { return GetAdjustedTime(); }
void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); }
Expand Down
5 changes: 1 addition & 4 deletions src/interfaces/chain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,9 +213,6 @@ class Chain
//! Check if the node is ready to broadcast transactions.
virtual bool isReadyToBroadcast() = 0;
Comment on lines 213 to 214
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "Remove Chain::isReadyToBroadcast method" (a5a6748)

Implementation and meaning of isReadyToBroadcast method is changing a lot, so I would rename it to something like "isInitializing" or "isLoading" and update the comment.

Also commit title could be changed to mention removing isInitialBlockDownload


//! Check if in IBD.
virtual bool isInitialBlockDownload() = 0;

//! Check if shutdown requested.
virtual bool shutdownRequested() = 0;

Expand Down Expand Up @@ -243,7 +240,7 @@ class Chain
virtual void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason) {}
virtual void blockConnected(const CBlock& block, int height) {}
virtual void blockDisconnected(const CBlock& block, int height) {}
virtual void updatedBlockTip() {}
virtual void updatedBlockTip(bool is_ibd) {}
virtual void chainStateFlushed(const CBlockLocator& locator) {}
};

Expand Down
16 changes: 10 additions & 6 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1177,9 +1177,11 @@ void CWallet::blockDisconnected(const CBlock& block, int height)
}
}

void CWallet::updatedBlockTip()
void CWallet::updatedBlockTip(bool is_ibd)
{
m_best_block_time = GetTime();
LOCK(cs_wallet);
m_is_ibd = is_ibd;
}


Expand Down Expand Up @@ -2014,6 +2016,8 @@ void CWallet::ResendWalletTransactions()
{ // cs_wallet scope
LOCK(cs_wallet);

if (m_is_ibd) return;
Copy link
Member

@jonatack jonatack Nov 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps make this change in the commit "Add m_is_ibd in CWallet` (or merge the two commits, IIUC)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #17484 (comment)

In commit "Remove Chain::isReadyToBroadcast method" (a5a6748)

Perhaps make this change in the commit "Add m_is_ibd in CWallet` (or merge the two commits, IIUC)

Change does make sense (and is necessary) in this commit due to isReadyToBroadcast call a few lines up. But it would be good to add a comment here mentioning this is also needed to avoid spamming nodes with old transactions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commit "[wallet] Add m_is_ibd in CWallet" (797ec9d)

I do think it would be helpful to have comment here saying this is skipped during ibd to avoid spam.

Also the comment above for isReadyToBroadcast() is incorrect after commit "[interfaces] Remove Chain::isReadyToBroadcast" (20eb0da), and should be updated to not mention IBD since the IBD check isn't there anymore


// Relay transactions
for (std::pair<const uint256, CWalletTx>& item : mapWallet) {
CWalletTx& wtx = item.second;
Expand Down Expand Up @@ -2587,9 +2591,9 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC
return true;
}

static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256& block_hash)
static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, bool is_ibd, const uint256& block_hash)
{
if (chain.isInitialBlockDownload()) {
if (is_ibd) {
return false;
}
constexpr int64_t MAX_ANTI_FEE_SNIPING_TIP_AGE = 8 * 60 * 60; // in seconds
Expand All @@ -2605,7 +2609,7 @@ static bool IsCurrentForAntiFeeSniping(interfaces::Chain& chain, const uint256&
* Return a height-based locktime for new transactions (uses the height of the
* current chain tip unless we are not synced with the current chain
*/
static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, const uint256& block_hash, int block_height)
static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, bool is_ibd, const uint256& block_hash, int block_height)
{
uint32_t locktime;
// Discourage fee sniping.
Expand All @@ -2628,7 +2632,7 @@ static uint32_t GetLocktimeForNewTransaction(interfaces::Chain& chain, const uin
// enough, that fee sniping isn't a problem yet, but by implementing a fix
// now we ensure code won't be written that makes assumptions about
// nLockTime that preclude a fix later.
if (IsCurrentForAntiFeeSniping(chain, block_hash)) {
if (IsCurrentForAntiFeeSniping(chain, is_ibd, block_hash)) {
locktime = block_height;

// Secondly occasionally randomly pick a nLockTime even further back, so
Expand Down Expand Up @@ -2707,7 +2711,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
{
std::set<CInputCoin> setCoins;
LOCK(cs_wallet);
txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight());
txNew.nLockTime = GetLocktimeForNewTransaction(chain(), m_is_ibd, GetLastBlockHash(), GetLastBlockHeight());
{
std::vector<COutput> vAvailableCoins;
AvailableCoins(vAvailableCoins, true, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0);
Expand Down
8 changes: 7 additions & 1 deletion src/wallet/wallet.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,12 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
// ScriptPubKeyMan::GetID. In many cases it will be the hash of an internal structure
std::map<uint256, std::unique_ptr<ScriptPubKeyMan>> m_spk_managers;

/* Flag to indicate chain is still in initial block download. Initialized to false,
* latched to updateBlockTip if node evaluates being out of IBD. This method MUST be
* called by rescanning code while boostraping the wallet.
*/
bool m_is_ibd GUARDED_BY(cs_wallet) = false;

public:
/*
* Main wallet lock.
Expand Down Expand Up @@ -902,7 +908,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
void transactionAddedToMempool(const CTransactionRef& tx) override;
void blockConnected(const CBlock& block, int height) override;
void blockDisconnected(const CBlock& block, int height) override;
void updatedBlockTip() override;
void updatedBlockTip(bool is_ibd) override;
int64_t RescanFromTime(int64_t startTime, const WalletRescanReserver& reserver, bool update);

struct ScanResult {
Expand Down