phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-25 18:53:23 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge bitcoin/bitcoin#21353: interfaces: Stop exposing wallet destdata to gui

Browse source 
f5ba424cd4 wallet: Add IsAddressUsed / SetAddressUsed methods (Russell Yanofsky)
62252c95e5 interfaces: Stop exposing wallet destdata to gui (Russell Yanofsky)
985430d9b2 test: Add gui test for wallet receive requests (Russell Yanofsky)

Pull request description:

  Stop giving GUI access to destdata rows in database. Replace with narrow API just for saving and reading receive request information.

  This simplifies code and should prevent the GUI from interfering with other destdata like address-used status. It also adds some more GUI test coverage.

  There are no changes in behavior.

ACKs for top commit:
  jarolrod:
    tACK f5ba424cd4
  laanwj:
    Code review ACK f5ba424cd4

Tree-SHA512: 5423df4786e537a59013cb5bfb9e1bc29a7ca4b8835360c00cc2165a59f925fdc355907a4ceb8bca0285bb4946ba235bffa7645537a951ad03fd3b4cee17b6b0
This commit is contained in:
W. J. van der Laan 2021-06-03 15:56:43 +02:00
commit 907d636e5e
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
9 changed files with 82 additions and 83 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
src/interfaces/wallet.h
View file

@ -112,14 +112,11 @@ public:
//! Get wallet address list.
virtual std::vector<WalletAddress> getAddresses() = 0;
//! Add dest data.
virtual bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) = 0;
//! Get receive requests.
virtual std::vector<std::string> getAddressReceiveRequests() = 0;
//! Erase dest data.
virtual bool eraseDestData(const CTxDestination& dest, const std::string& key) = 0;
//! Get dest values with prefix.
virtual std::vector<std::string> getDestValues(const std::string& prefix) = 0;
//! Save or remove receive request.
virtual bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) = 0;
//! Lock coin.
virtual void lockCoin(const COutPoint& output) = 0;

12
src/qt/recentrequeststablemodel.cpp
View file

@ -10,7 +10,10 @@
#include <qt/walletmodel.h>
#include <clientversion.h>
#include <interfaces/wallet.h>
#include <key_io.h>
#include <streams.h>
#include <util/string.h>
#include <utility>
@ -21,10 +24,9 @@ RecentRequestsTableModel::RecentRequestsTableModel(WalletModel *parent) :
QAbstractTableModel(parent), walletModel(parent)
{
// Load entries from wallet
std::vector<std::string> vReceiveRequests;
parent->loadReceiveRequests(vReceiveRequests);
for (const std::string& request : vReceiveRequests)
for (const std::string& request : parent->wallet().getAddressReceiveRequests()) {
addNewRequest(request);
}
/* These columns must match the indices in the ColumnIndex enumeration */
columns << tr("Date") << tr("Label") << tr("Message") << getAmountTitle();
@ -150,7 +152,7 @@ bool RecentRequestsTableModel::removeRows(int row, int count, const QModelIndex
for (int i = 0; i < count; ++i)
{
const RecentRequestEntry* rec = &list[row+i];
if (!walletModel->saveReceiveRequest(rec->recipient.address.toStdString(), rec->id, ""))
if (!walletModel->wallet().setAddressReceiveRequest(DecodeDestination(rec->recipient.address.toStdString()), ToString(rec->id), ""))
return false;
}
@ -179,7 +181,7 @@ void RecentRequestsTableModel::addNewRequest(const SendCoinsRecipient &recipient
CDataStream ss(SER_DISK, CLIENT_VERSION);
ss << newEntry;
if (!walletModel->saveReceiveRequest(recipient.address.toStdString(), newEntry.id, ss.str()))
if (!walletModel->wallet().setAddressReceiveRequest(DecodeDestination(recipient.address.toStdString()), ToString(newEntry.id), ss.str()))
return;
addNewRequest(newEntry);

22
src/qt/test/wallettests.cpp
View file

@ -224,6 +224,7 @@ void TestGUI(interfaces::Node& node)
int initialRowCount = requestTableModel->rowCount({});
QPushButton* requestPaymentButton = receiveCoinsDialog.findChild<QPushButton*>("receiveButton");
requestPaymentButton->click();
QString address;
for (QWidget* widget : QApplication::topLevelWidgets()) {
if (widget->inherits("ReceiveRequestDialog")) {
ReceiveRequestDialog* receiveRequestDialog = qobject_cast<ReceiveRequestDialog*>(widget);
@ -232,6 +233,9 @@ void TestGUI(interfaces::Node& node)
QString uri = receiveRequestDialog->QObject::findChild<QLabel*>("uri_content")->text();
QCOMPARE(uri.count("bitcoin:"), 2);
QCOMPARE(receiveRequestDialog->QObject::findChild<QLabel*>("address_tag")->text(), QString("Address:"));
QVERIFY(address.isEmpty());
address = receiveRequestDialog->QObject::findChild<QLabel*>("address_content")->text();
QVERIFY(!address.isEmpty());
QCOMPARE(uri.count("amount=0.00000001"), 2);
QCOMPARE(receiveRequestDialog->QObject::findChild<QLabel*>("amount_tag")->text(), QString("Amount:"));
@ -258,12 +262,30 @@ void TestGUI(interfaces::Node& node)
int currentRowCount = requestTableModel->rowCount({});
QCOMPARE(currentRowCount, initialRowCount+1);
// Check addition to wallet
std::vector<std::string> requests = walletModel.wallet().getAddressReceiveRequests();
QCOMPARE(requests.size(), size_t{1});
RecentRequestEntry entry;
CDataStream{MakeUCharSpan(requests[0]), SER_DISK, CLIENT_VERSION} >> entry;
QCOMPARE(entry.nVersion, int{1});
QCOMPARE(entry.id, int64_t{1});
QVERIFY(entry.date.isValid());
QCOMPARE(entry.recipient.address, address);
QCOMPARE(entry.recipient.label, QString{"TEST_LABEL_1"});
QCOMPARE(entry.recipient.amount, CAmount{1});
QCOMPARE(entry.recipient.message, QString{"TEST_MESSAGE_1"});
QCOMPARE(entry.recipient.sPaymentRequest, std::string{});
QCOMPARE(entry.recipient.authenticatedMerchant, QString{});
// Check Remove button
QTableView* table = receiveCoinsDialog.findChild<QTableView*>("recentRequestsView");
table->selectRow(currentRowCount-1);
QPushButton* removeRequestButton = receiveCoinsDialog.findChild<QPushButton*>("removeRequestButton");
removeRequestButton->click();
QCOMPARE(requestTableModel->rowCount({}), currentRowCount-1);
// Check removal from wallet
QCOMPARE(walletModel.wallet().getAddressReceiveRequests().size(), size_t{0});
}
} // namespace

19
src/qt/walletmodel.cpp
View file

@ -466,25 +466,6 @@ void WalletModel::UnlockContext::CopyFrom(UnlockContext&& rhs)
rhs.relock = false;
}
void WalletModel::loadReceiveRequests(std::vector<std::string>& vReceiveRequests)
{
vReceiveRequests = m_wallet->getDestValues("rr"); // receive request
}
bool WalletModel::saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest)
{
CTxDestination dest = DecodeDestination(sAddress);
std::stringstream ss;
ss << nId;
std::string key = "rr" + ss.str(); // "rr" prefix = "receive request" in destdata
if (sRequest.empty())
return m_wallet->eraseDestData(dest, key);
else
return m_wallet->addDestData(dest, key, sRequest);
}
bool WalletModel::bumpFee(uint256 hash, uint256& new_hash)
{
CCoinControl coin_control;

3
src/qt/walletmodel.h
View file

@ -135,9 +135,6 @@ public:
UnlockContext requestUnlock();
void loadReceiveRequests(std::vector<std::string>& vReceiveRequests);
bool saveReceiveRequest(const std::string &sAddress, const int64_t nId, const std::string &sRequest);
bool bumpFee(uint256 hash, uint256& new_hash);
static bool isWalletEnabled();

20
src/wallet/interfaces.cpp
View file

@ -197,22 +197,14 @@ public:
}
return result;
}
bool addDestData(const CTxDestination& dest, const std::string& key, const std::string& value) override
{
std::vector<std::string> getAddressReceiveRequests() override {
LOCK(m_wallet->cs_wallet);
return m_wallet->GetAddressReceiveRequests();
}
bool setAddressReceiveRequest(const CTxDestination& dest, const std::string& id, const std::string& value) override {
LOCK(m_wallet->cs_wallet);
WalletBatch batch{m_wallet->GetDatabase()};
return m_wallet->AddDestData(batch, dest, key, value);
}
bool eraseDestData(const CTxDestination& dest, const std::string& key) override
{
LOCK(m_wallet->cs_wallet);
WalletBatch batch{m_wallet->GetDatabase()};
return m_wallet->EraseDestData(batch, dest, key);
}
std::vector<std::string> getDestValues(const std::string& prefix) override
{
LOCK(m_wallet->cs_wallet);
return m_wallet->GetDestValues(prefix);
return m_wallet->SetAddressReceiveRequest(batch, dest, id, value);
}
void lockCoin(const COutPoint& output) override
{

8
src/wallet/test/wallet_tests.cpp
View file

@ -385,11 +385,11 @@ BOOST_AUTO_TEST_CASE(LoadReceiveRequests)
CTxDestination dest = PKHash();
LOCK(m_wallet.cs_wallet);
WalletBatch batch{m_wallet.GetDatabase()};
m_wallet.AddDestData(batch, dest, "misc", "val_misc");
m_wallet.AddDestData(batch, dest, "rr0", "val_rr0");
m_wallet.AddDestData(batch, dest, "rr1", "val_rr1");
m_wallet.SetAddressUsed(batch, dest, true);
m_wallet.SetAddressReceiveRequest(batch, dest, "0", "val_rr0");
m_wallet.SetAddressReceiveRequest(batch, dest, "1", "val_rr1");
auto values = m_wallet.GetDestValues("rr");
auto values = m_wallet.GetAddressReceiveRequests();
BOOST_CHECK_EQUAL(values.size(), 2U);
BOOST_CHECK_EQUAL(values[0], "val_rr0");
BOOST_CHECK_EQUAL(values[1], "val_rr1");

53
src/wallet/wallet.cpp
View file

@ -814,12 +814,11 @@ void CWallet::SetSpentKeyState(WalletBatch& batch, const uint256& hash, unsigned
CTxDestination dst;
if (ExtractDestination(srctx->tx->vout[n].scriptPubKey, dst)) {
if (IsMine(dst)) {
if (used && !GetDestData(dst, "used", nullptr)) {
if (AddDestData(batch, dst, "used", "p")) { // p for "present", opposite of absent (null)
if (used != IsAddressUsed(dst)) {
if (used) {
tx_destinations.insert(dst);
}
} else if (!used && GetDestData(dst, "used", nullptr)) {
EraseDestData(batch, dst, "used");
SetAddressUsed(batch, dst, used);
}
}
}
@ -835,7 +834,7 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
if (!ExtractDestination(srctx->tx->vout[n].scriptPubKey, dest)) {
return false;
}
if (GetDestData(dest, "used", nullptr)) {
if (IsAddressUsed(dest)) {
return true;
}
if (IsLegacy()) {
@ -843,15 +842,15 @@ bool CWallet::IsSpentKey(const uint256& hash, unsigned int n) const
assert(spk_man != nullptr);
for (const auto& keyid : GetAffectedKeys(srctx->tx->vout[n].scriptPubKey, *spk_man)) {
WitnessV0KeyHash wpkh_dest(keyid);
if (GetDestData(wpkh_dest, "used", nullptr)) {
if (IsAddressUsed(wpkh_dest)) {
return true;
}
ScriptHash sh_wpkh_dest(GetScriptForDestination(wpkh_dest));
if (GetDestData(sh_wpkh_dest, "used", nullptr)) {
if (IsAddressUsed(sh_wpkh_dest)) {
return true;
}
PKHash pkh_dest(keyid);
if (GetDestData(pkh_dest, "used", nullptr)) {
if (IsAddressUsed(pkh_dest)) {
return true;
}
}
@ -2387,45 +2386,45 @@ unsigned int CWallet::ComputeTimeSmart(const CWalletTx& wtx) const
return nTimeSmart;
}
bool CWallet::AddDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key, const std::string &value)
bool CWallet::SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used)
{
const std::string key{"used"};
if (std::get_if<CNoDestination>(&dest))
return false;
if (!used) {
if (auto* data = util::FindKey(m_address_book, dest)) data->destdata.erase(key);
return batch.EraseDestData(EncodeDestination(dest), key);
}
const std::string value{"1"};
m_address_book[dest].destdata.insert(std::make_pair(key, value));
return batch.WriteDestData(EncodeDestination(dest), key, value);
}
bool CWallet::EraseDestData(WalletBatch& batch, const CTxDestination &dest, const std::string &key)
{
if (!m_address_book[dest].destdata.erase(key))
return false;
return batch.EraseDestData(EncodeDestination(dest), key);
}
void CWallet::LoadDestData(const CTxDestination &dest, const std::string &key, const std::string &value)
{
m_address_book[dest].destdata.insert(std::make_pair(key, value));
}
bool CWallet::GetDestData(const CTxDestination &dest, const std::string &key, std::string *value) const
bool CWallet::IsAddressUsed(const CTxDestination& dest) const
{
const std::string key{"used"};
std::map<CTxDestination, CAddressBookData>::const_iterator i = m_address_book.find(dest);
if(i != m_address_book.end())
{
CAddressBookData::StringMap::const_iterator j = i->second.destdata.find(key);
if(j != i->second.destdata.end())
{
if(value)
*value = j->second;
return true;
}
}
return false;
}
std::vector<std::string> CWallet::GetDestValues(const std::string& prefix) const
std::vector<std::string> CWallet::GetAddressReceiveRequests() const
{
const std::string prefix{"rr"};
std::vector<std::string> values;
for (const auto& address : m_address_book) {
for (const auto& data : address.second.destdata) {
@ -2437,6 +2436,20 @@ std::vector<std::string> CWallet::GetDestValues(const std::string& prefix) const
return values;
}
bool CWallet::SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value)
{
const std::string key{"rr" + id}; // "rr" prefix = "receive request" in destdata
CAddressBookData& data = m_address_book.at(dest);
if (value.empty()) {
if (!batch.EraseDestData(EncodeDestination(dest), key)) return false;
data.destdata.erase(key);
} else {
if (!batch.WriteDestData(EncodeDestination(dest), key, value)) return false;
data.destdata[key] = value;
}
return true;
}
std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error_string)
{
// Do some checking on wallet path. It should be either a:

17
src/wallet/wallet.h
View file

@ -479,19 +479,8 @@ public:
bool LoadMinVersion(int nVersion) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); nWalletVersion = nVersion; return true; }
/**
* Adds a destination data tuple to the store, and saves it to disk
* When adding new fields, take care to consider how DelAddressBook should handle it!
*/
bool AddDestData(WalletBatch& batch, const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
//! Erases a destination data tuple in the store and on disk
bool EraseDestData(WalletBatch& batch, const CTxDestination& dest, const std::string& key) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
//! Adds a destination data tuple to the store, without saving it to disk
void LoadDestData(const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
//! Look up a destination data tuple in the store, return true if found false otherwise
bool GetDestData(const CTxDestination& dest, const std::string& key, std::string* value) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
//! Get all destination values matching a prefix.
std::vector<std::string> GetDestValues(const std::string& prefix) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
//! Holds a timestamp at which point the wallet is scheduled (externally) to be relocked. Caller must arrange for actual relocking to occur via Lock().
int64_t nRelockTime GUARDED_BY(cs_wallet){0};
@ -705,6 +694,12 @@ public:
bool DelAddressBook(const CTxDestination& address);
bool IsAddressUsed(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
bool SetAddressUsed(WalletBatch& batch, const CTxDestination& dest, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
std::vector<std::string> GetAddressReceiveRequests() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
bool SetAddressReceiveRequest(WalletBatch& batch, const CTxDestination& dest, const std::string& id, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
unsigned int GetKeyPoolSize() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
//! signify that a particular wallet feature is now used.