diff options
-rw-r--r-- | src/bench/verify_script.cpp | 2 | ||||
-rw-r--r-- | src/coins.cpp | 7 | ||||
-rw-r--r-- | src/dbwrapper.h | 12 | ||||
-rw-r--r-- | src/streams.h | 6 | ||||
-rw-r--r-- | src/test/coins_tests.cpp | 28 | ||||
-rw-r--r-- | src/wallet/db.cpp | 8 | ||||
-rw-r--r-- | src/wallet/db.h | 12 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 31 | ||||
-rw-r--r-- | src/wallet/wallet.h | 4 |
9 files changed, 76 insertions, 34 deletions
diff --git a/src/bench/verify_script.cpp b/src/bench/verify_script.cpp index bf04f90e0e..23bbadc88d 100644 --- a/src/bench/verify_script.cpp +++ b/src/bench/verify_script.cpp @@ -93,7 +93,7 @@ static void VerifyScriptBench(benchmark::State& state) txCredit.vout[0].scriptPubKey.data(), txCredit.vout[0].scriptPubKey.size(), txCredit.vout[0].nValue, - (const unsigned char*)&stream[0], stream.size(), 0, flags, nullptr); + (const unsigned char*)stream.data(), stream.size(), 0, flags, nullptr); assert(csuccess == 1); #endif } diff --git a/src/coins.cpp b/src/coins.cpp index 68f32a9da4..4d0e4bc0ad 100644 --- a/src/coins.cpp +++ b/src/coins.cpp @@ -207,6 +207,13 @@ bool CCoinsViewCache::BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn entry.flags |= CCoinsCacheEntry::FRESH; } } else { + // Assert that the child cache entry was not marked FRESH if the + // parent cache entry has unspent outputs. If this ever happens, + // it means the FRESH flag was misapplied and there is a logic + // error in the calling code. + if ((it->second.flags & CCoinsCacheEntry::FRESH) && !itUs->second.coins.IsPruned()) + throw std::logic_error("FRESH flag misapplied to cache entry for base transaction with spendable outputs"); + // Found the entry in the parent cache if ((itUs->second.flags & CCoinsCacheEntry::FRESH) && it->second.coins.IsPruned()) { // The grandparent does not have an entry, and the child is diff --git a/src/dbwrapper.h b/src/dbwrapper.h index 787e2608db..dd59cc00ff 100644 --- a/src/dbwrapper.h +++ b/src/dbwrapper.h @@ -67,12 +67,12 @@ public: { ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE); ssKey << key; - leveldb::Slice slKey(&ssKey[0], ssKey.size()); + leveldb::Slice slKey(ssKey.data(), ssKey.size()); ssValue.reserve(DBWRAPPER_PREALLOC_VALUE_SIZE); ssValue << value; ssValue.Xor(dbwrapper_private::GetObfuscateKey(parent)); - leveldb::Slice slValue(&ssValue[0], ssValue.size()); + leveldb::Slice slValue(ssValue.data(), ssValue.size()); batch.Put(slKey, slValue); ssKey.clear(); @@ -84,7 +84,7 @@ public: { ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE); ssKey << key; - leveldb::Slice slKey(&ssKey[0], ssKey.size()); + leveldb::Slice slKey(ssKey.data(), ssKey.size()); batch.Delete(slKey); ssKey.clear(); @@ -115,7 +115,7 @@ public: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE); ssKey << key; - leveldb::Slice slKey(&ssKey[0], ssKey.size()); + leveldb::Slice slKey(ssKey.data(), ssKey.size()); piter->Seek(slKey); } @@ -208,7 +208,7 @@ public: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE); ssKey << key; - leveldb::Slice slKey(&ssKey[0], ssKey.size()); + leveldb::Slice slKey(ssKey.data(), ssKey.size()); std::string strValue; leveldb::Status status = pdb->Get(readoptions, slKey, &strValue); @@ -242,7 +242,7 @@ public: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(DBWRAPPER_PREALLOC_KEY_SIZE); ssKey << key; - leveldb::Slice slKey(&ssKey[0], ssKey.size()); + leveldb::Slice slKey(ssKey.data(), ssKey.size()); std::string strValue; leveldb::Status status = pdb->Get(readoptions, slKey, &strValue); diff --git a/src/streams.h b/src/streams.h index 471602227a..1d3b55c91e 100644 --- a/src/streams.h +++ b/src/streams.h @@ -174,12 +174,10 @@ public: Init(nTypeIn, nVersionIn); } -#if !defined(_MSC_VER) || _MSC_VER >= 1300 CDataStream(const char* pbegin, const char* pend, int nTypeIn, int nVersionIn) : vch(pbegin, pend) { Init(nTypeIn, nVersionIn); } -#endif CDataStream(const vector_type& vchIn, int nTypeIn, int nVersionIn) : vch(vchIn.begin(), vchIn.end()) { @@ -245,6 +243,8 @@ public: void clear() { vch.clear(); nReadPos = 0; } iterator insert(iterator it, const char& x=char()) { return vch.insert(it, x); } void insert(iterator it, size_type n, const char& x) { vch.insert(it, n, x); } + value_type* data() { return vch.data() + nReadPos; } + const value_type* data() const { return vch.data() + nReadPos; } void insert(iterator it, std::vector<char>::const_iterator first, std::vector<char>::const_iterator last) { @@ -259,7 +259,6 @@ public: vch.insert(it, first, last); } -#if !defined(_MSC_VER) || _MSC_VER >= 1300 void insert(iterator it, const char* first, const char* last) { assert(last - first >= 0); @@ -272,7 +271,6 @@ public: else vch.insert(it, first, last); } -#endif iterator erase(iterator it) { diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp index 70b958c296..b25c7ccc51 100644 --- a/src/test/coins_tests.cpp +++ b/src/test/coins_tests.cpp @@ -791,12 +791,18 @@ BOOST_AUTO_TEST_CASE(ccoins_modify_new) void CheckWriteCoins(CAmount parent_value, CAmount child_value, CAmount expected_value, char parent_flags, char child_flags, char expected_flags) { SingleEntryCacheTest test(ABSENT, parent_value, parent_flags); - WriteCoinsViewEntry(test.cache, child_value, child_flags); - test.cache.SelfTest(); CAmount result_value; char result_flags; - GetCoinsMapEntry(test.cache.map(), result_value, result_flags); + try { + WriteCoinsViewEntry(test.cache, child_value, child_flags); + test.cache.SelfTest(); + GetCoinsMapEntry(test.cache.map(), result_value, result_flags); + } catch (std::logic_error& e) { + result_value = FAIL; + result_flags = NO_ENTRY; + } + BOOST_CHECK_EQUAL(result_value, expected_value); BOOST_CHECK_EQUAL(result_flags, expected_flags); } @@ -840,21 +846,21 @@ BOOST_AUTO_TEST_CASE(ccoins_write) CheckWriteCoins(VALUE1, ABSENT, VALUE1, DIRTY , NO_ENTRY , DIRTY ); CheckWriteCoins(VALUE1, ABSENT, VALUE1, DIRTY|FRESH, NO_ENTRY , DIRTY|FRESH); CheckWriteCoins(VALUE1, PRUNED, PRUNED, 0 , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, PRUNED, PRUNED, 0 , DIRTY|FRESH, DIRTY ); + CheckWriteCoins(VALUE1, PRUNED, FAIL , 0 , DIRTY|FRESH, NO_ENTRY ); CheckWriteCoins(VALUE1, PRUNED, ABSENT, FRESH , DIRTY , NO_ENTRY ); - CheckWriteCoins(VALUE1, PRUNED, ABSENT, FRESH , DIRTY|FRESH, NO_ENTRY ); + CheckWriteCoins(VALUE1, PRUNED, FAIL , FRESH , DIRTY|FRESH, NO_ENTRY ); CheckWriteCoins(VALUE1, PRUNED, PRUNED, DIRTY , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, PRUNED, PRUNED, DIRTY , DIRTY|FRESH, DIRTY ); + CheckWriteCoins(VALUE1, PRUNED, FAIL , DIRTY , DIRTY|FRESH, NO_ENTRY ); CheckWriteCoins(VALUE1, PRUNED, ABSENT, DIRTY|FRESH, DIRTY , NO_ENTRY ); - CheckWriteCoins(VALUE1, PRUNED, ABSENT, DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY ); + CheckWriteCoins(VALUE1, PRUNED, FAIL , DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY ); CheckWriteCoins(VALUE1, VALUE2, VALUE2, 0 , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, 0 , DIRTY|FRESH, DIRTY ); + CheckWriteCoins(VALUE1, VALUE2, FAIL , 0 , DIRTY|FRESH, NO_ENTRY ); CheckWriteCoins(VALUE1, VALUE2, VALUE2, FRESH , DIRTY , DIRTY|FRESH); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, FRESH , DIRTY|FRESH, DIRTY|FRESH); + CheckWriteCoins(VALUE1, VALUE2, FAIL , FRESH , DIRTY|FRESH, NO_ENTRY ); CheckWriteCoins(VALUE1, VALUE2, VALUE2, DIRTY , DIRTY , DIRTY ); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, DIRTY , DIRTY|FRESH, DIRTY ); + CheckWriteCoins(VALUE1, VALUE2, FAIL , DIRTY , DIRTY|FRESH, NO_ENTRY ); CheckWriteCoins(VALUE1, VALUE2, VALUE2, DIRTY|FRESH, DIRTY , DIRTY|FRESH); - CheckWriteCoins(VALUE1, VALUE2, VALUE2, DIRTY|FRESH, DIRTY|FRESH, DIRTY|FRESH); + CheckWriteCoins(VALUE1, VALUE2, FAIL , DIRTY|FRESH, DIRTY|FRESH, NO_ENTRY ); // The checks above omit cases where the child flags are not DIRTY, since // they would be too repetitive (the parent cache is never updated in these diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 4ae5e80236..800e1f4e29 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -397,15 +397,15 @@ bool CDB::Rewrite(const string& strFile, const char* pszSkip) break; } if (pszSkip && - strncmp(&ssKey[0], pszSkip, std::min(ssKey.size(), strlen(pszSkip))) == 0) + strncmp(ssKey.data(), pszSkip, std::min(ssKey.size(), strlen(pszSkip))) == 0) continue; - if (strncmp(&ssKey[0], "\x07version", 8) == 0) { + if (strncmp(ssKey.data(), "\x07version", 8) == 0) { // Update version: ssValue.clear(); ssValue << CLIENT_VERSION; } - Dbt datKey(&ssKey[0], ssKey.size()); - Dbt datValue(&ssValue[0], ssValue.size()); + Dbt datKey(ssKey.data(), ssKey.size()); + Dbt datValue(ssValue.data(), ssValue.size()); int ret2 = pdbCopy->put(NULL, &datKey, &datValue, DB_NOOVERWRITE); if (ret2 > 0) fSuccess = false; diff --git a/src/wallet/db.h b/src/wallet/db.h index b78f0f092a..bc15f2147f 100644 --- a/src/wallet/db.h +++ b/src/wallet/db.h @@ -122,7 +122,7 @@ protected: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(1000); ssKey << key; - Dbt datKey(&ssKey[0], ssKey.size()); + Dbt datKey(ssKey.data(), ssKey.size()); // Read Dbt datValue; @@ -158,13 +158,13 @@ protected: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(1000); ssKey << key; - Dbt datKey(&ssKey[0], ssKey.size()); + Dbt datKey(ssKey.data(), ssKey.size()); // Value CDataStream ssValue(SER_DISK, CLIENT_VERSION); ssValue.reserve(10000); ssValue << value; - Dbt datValue(&ssValue[0], ssValue.size()); + Dbt datValue(ssValue.data(), ssValue.size()); // Write int ret = pdb->put(activeTxn, &datKey, &datValue, (fOverwrite ? 0 : DB_NOOVERWRITE)); @@ -187,7 +187,7 @@ protected: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(1000); ssKey << key; - Dbt datKey(&ssKey[0], ssKey.size()); + Dbt datKey(ssKey.data(), ssKey.size()); // Erase int ret = pdb->del(activeTxn, &datKey, 0); @@ -207,7 +207,7 @@ protected: CDataStream ssKey(SER_DISK, CLIENT_VERSION); ssKey.reserve(1000); ssKey << key; - Dbt datKey(&ssKey[0], ssKey.size()); + Dbt datKey(ssKey.data(), ssKey.size()); // Exists int ret = pdb->exists(activeTxn, &datKey, 0); @@ -234,7 +234,7 @@ protected: Dbt datKey; unsigned int fFlags = DB_NEXT; if (setRange) { - datKey.set_data(&ssKey[0]); + datKey.set_data(ssKey.data()); datKey.set_size(ssKey.size()); fFlags = DB_SET_RANGE; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ff7a03bc55..2775f4def3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2534,8 +2534,37 @@ bool CWallet::CreateTransaction(const vector<CRecipient>& vecSend, CWalletTx& wt return false; } - if (nFeeRet >= nFeeNeeded) + if (nFeeRet >= nFeeNeeded) { + // Reduce fee to only the needed amount if we have change + // output to increase. This prevents potential overpayment + // in fees if the coins selected to meet nFeeNeeded result + // in a transaction that requires less fee than the prior + // iteration. + // TODO: The case where nSubtractFeeFromAmount > 0 remains + // to be addressed because it requires returning the fee to + // the payees and not the change output. + // TODO: The case where there is no change output remains + // to be addressed so we avoid creating too small an output. + if (nFeeRet > nFeeNeeded && nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) { + CAmount extraFeePaid = nFeeRet - nFeeNeeded; + vector<CTxOut>::iterator change_position = txNew.vout.begin()+nChangePosInOut; + change_position->nValue += extraFeePaid; + nFeeRet -= extraFeePaid; + } break; // Done, enough fee included. + } + + // Try to reduce change to include necessary fee + if (nChangePosInOut != -1 && nSubtractFeeFromAmount == 0) { + CAmount additionalFeeNeeded = nFeeNeeded - nFeeRet; + vector<CTxOut>::iterator change_position = txNew.vout.begin()+nChangePosInOut; + // Only reduce change if remaining amount is still a large enough output. + if (change_position->nValue >= MIN_FINAL_CHANGE + additionalFeeNeeded) { + change_position->nValue -= additionalFeeNeeded; + nFeeRet += additionalFeeNeeded; + break; // Done, able to increase fee from change + } + } // Include more fee and try again. nFeeRet = nFeeNeeded; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 1d1f84ebb9..b9fa6bb24b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -48,8 +48,10 @@ static const CAmount DEFAULT_TRANSACTION_FEE = 0; static const CAmount DEFAULT_FALLBACK_FEE = 20000; //! -mintxfee default static const CAmount DEFAULT_TRANSACTION_MINFEE = 1000; -//! minimum change amount +//! target minimum change amount static const CAmount MIN_CHANGE = CENT; +//! final minimum change amount after paying for fees +static const CAmount MIN_FINAL_CHANGE = MIN_CHANGE/2; //! Default for -spendzeroconfchange static const bool DEFAULT_SPEND_ZEROCONF_CHANGE = true; //! Default for -sendfreetransactions |