Skip to content

Conversation

@codablock
Copy link

Continuation of #1691

This PR contains the actual per-utxo patch (bitcoin#10195) and fixes which came later in Bitcoin.

With this PR, Dash will do a one-time db-upgrade to the new schema on first start. While this happens, dash-qt will show the progress on the loading screen. This progress is currently badly positioned. If it's ok for you all, I'll fix this in a later PR.

@codablock codablock force-pushed the backport_bitcoin_perutxo branch from da5217f to 83945a6 Compare October 26, 2017 21:31
@codablock
Copy link
Author

Hmm, I got compilation errors in most of the Travis jobs. On my laptop it compiles just fine.

This is most likely due to gcc version differences. Does anyone know which gcc version is used in Travis builds? @flare maybe?

@UdjinM6 UdjinM6 added this to the 12.2.1 milestone Oct 26, 2017
@schinzelh
Copy link

schinzelh commented Oct 27, 2017

@codablock I'll check against gitian as well, Travis-CI throws this

coins.h:119:14: error:   initializing argument 1 of ‘CCoinsCacheEntry::CCoinsCacheEntry(Coin&&)’
     explicit CCoinsCacheEntry(Coin&& coin_) : coin(std::move(coin_)), flags(0) {}
              ^

@schinzelh
Copy link

schinzelh commented Oct 27, 2017

gitian confirms compilation error:

In file included from /usr/include/x86_64-linux-gnu/c++/4.8/32/bits/c++allocator.h:33:0,
                 from /usr/include/c++/4.8/bits/allocator.h:46,
                 from /usr/include/c++/4.8/string:41,
                 from /usr/include/c++/4.8/random:41,
                 from /usr/include/c++/4.8/bits/stl_algo.h:65,
                 from /usr/include/c++/4.8/algorithm:62,
                 from ./serialize.h:11,
                 from ./amount.h:9,
                 from primitives/transaction.h:9,
                 from compressor.h:9,
                 from coins.h:9,
                 from coins.cpp:5:
/usr/include/c++/4.8/ext/new_allocator.h: In instantiation of ‘void __gnu_cxx::new_allocator< <template-parameter-1-1> >::construct(_Up*, _Args&& ...) [with _Up = CCoinsCacheEntry; _Args = {Coin&}; _Tp = boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> >]’:
/usr/include/c++/4.8/bits/alloc_traits.h:254:4:   required from ‘static typename std::enable_if<std::allocator_traits<_Alloc>::__construct_helper<_Tp, _Args>::value, void>::type std::allocator_traits<_Alloc>::_S_construct(_Alloc&, _Tp*, _Args&& ...) [with _Tp = CCoinsCacheEntry; _Args = {Coin&}; _Alloc = std::allocator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >; typename std::enable_if<std::allocator_traits<_Alloc>::__construct_helper<_Tp, _Args>::value, void>::type = void]’
/usr/include/c++/4.8/bits/alloc_traits.h:393:57:   required from ‘static decltype (_S_construct(__a, __p, (forward<_Args>)(std::allocator_traits::construct::__args)...)) std::allocator_traits<_Alloc>::construct(_Alloc&, _Tp*, _Args&& ...) [with _Tp = CCoinsCacheEntry; _Args = {Coin&}; _Alloc = std::allocator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >; decltype (_S_construct(__a, __p, (forward<_Args>)(std::allocator_traits::construct::__args)...)) = <type error>]’
/home/ubuntu/build/dash/depends/i686-pc-linux-gnu/share/../include/boost/unordered/detail/allocate.hpp:917:51:   required from ‘void boost::unordered::detail::func::call_construct(Alloc&, T*, Args&& ...) [with Alloc = std::allocator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >; T = CCoinsCacheEntry; Args = {Coin&}]’
/home/ubuntu/build/dash/depends/i686-pc-linux-gnu/share/../include/boost/unordered/detail/allocate.hpp:1057:4:   required from ‘void boost::unordered::detail::func::construct_from_tuple(Alloc&, T*, const std::tuple<A0>&) [with Alloc = std::allocator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >; T = CCoinsCacheEntry; A0 = Coin&&]’
/home/ubuntu/build/dash/depends/i686-pc-linux-gnu/share/../include/boost/unordered/detail/allocate.hpp:1124:35:   required from ‘typename boost::enable_if<boost::unordered::detail::func::use_piecewise<A0>, void>::type boost::unordered::detail::func::construct_from_args(Alloc&, std::pair<_Tp1, _Tp2>*, A0&&, A1&&, A2&&) [with Alloc = std::allocator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >; A = const COutPoint; B = CCoinsCacheEntry; A0 = const std::piecewise_construct_t&; A1 = std::tuple<const COutPoint&>; A2 = std::tuple<Coin&&>; typename boost::enable_if<boost::unordered::detail::func::use_piecewise<A0>, void>::type = void]’
/home/ubuntu/build/dash/depends/i686-pc-linux-gnu/share/../include/boost/unordered/detail/allocate.hpp:1338:48:   required from ‘typename boost::unordered::detail::allocator_traits<Alloc>::pointer boost::unordered::detail::func::construct_node_from_args(Alloc&, Args&& ...) [with Alloc = std::allocator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >; Args = {const std::piecewise_construct_t&, std::tuple<const COutPoint&>, std::tuple<Coin&&>}; typename boost::unordered::detail::allocator_traits<Alloc>::pointer = boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> >*]’
/home/ubuntu/build/dash/depends/i686-pc-linux-gnu/share/../include/boost/unordered/detail/unique.hpp:424:80:   required from ‘boost::unordered::detail::table_impl<Types>::emplace_return boost::unordered::detail::table_impl<Types>::emplace_impl(const key_type&, Args&& ...) [with Args = {const std::piecewise_construct_t&, std::tuple<const COutPoint&>, std::tuple<Coin&&>}; Types = boost::unordered::detail::map<std::allocator<std::pair<const COutPoint, CCoinsCacheEntry> >, COutPoint, CCoinsCacheEntry, SaltedOutpointHasher, std::equal_to<COutPoint> >; boost::unordered::detail::table_impl<Types>::emplace_return = std::pair<boost::unordered::iterator_detail::iterator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >, bool>; typename boost::unordered::detail::table<Types>::iterator = boost::unordered::iterator_detail::iterator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >; boost::unordered::detail::table_impl<Types>::key_type = COutPoint]’
/home/ubuntu/build/dash/depends/i686-pc-linux-gnu/share/../include/boost/unordered/detail/unique.hpp:360:48:   required from ‘boost::unordered::detail::table_impl<Types>::emplace_return boost::unordered::detail::table_impl<Types>::emplace(Args&& ...) [with Args = {const std::piecewise_construct_t&, std::tuple<const COutPoint&>, std::tuple<Coin&&>}; Types = boost::unordered::detail::map<std::allocator<std::pair<const COutPoint, CCoinsCacheEntry> >, COutPoint, CCoinsCacheEntry, SaltedOutpointHasher, std::equal_to<COutPoint> >; boost::unordered::detail::table_impl<Types>::emplace_return = std::pair<boost::unordered::iterator_detail::iterator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >, bool>; typename boost::unordered::detail::table<Types>::iterator = boost::unordered::iterator_detail::iterator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >]’
/home/ubuntu/build/dash/depends/i686-pc-linux-gnu/share/../include/boost/unordered/unordered_map.hpp:269:64:   required from ‘std::pair<typename boost::unordered::detail::map<A, K, T, H, P>::table::iterator, bool> boost::unordered::unordered_map<K, T, H, P, A>::emplace(Args&& ...) [with Args = {const std::piecewise_construct_t&, std::tuple<const COutPoint&>, std::tuple<Coin&&>}; K = COutPoint; T = CCoinsCacheEntry; H = SaltedOutpointHasher; P = std::equal_to<COutPoint>; A = std::allocator<std::pair<const COutPoint, CCoinsCacheEntry> >; typename boost::unordered::detail::map<A, K, T, H, P>::table::iterator = boost::unordered::iterator_detail::iterator<boost::unordered::detail::ptr_node<std::pair<const COutPoint, CCoinsCacheEntry> > >]’
coins.cpp:48:146:   required from here
/usr/include/c++/4.8/ext/new_allocator.h:120:4: error: cannot bind ‘Coin’ lvalue to ‘Coin&&’
  { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
    ^
In file included from coins.cpp:5:0:
coins.h:119:14: error:   initializing argument 1 of ‘CCoinsCacheEntry::CCoinsCacheEntry(Coin&&)’
     explicit CCoinsCacheEntry(Coin&& coin_) : coin(std::move(coin_)), flags(0) {}
              ^

@schinzelh
Copy link

Sidenote: gitian is using gcc-4.8.4 on Ubuntu Trusty, same as Bitcoin 0.15 - so the issue must be somewhere in the code :)

ubuntu@gitian:~$ gcc --version
+ gcc --version
gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4
Copyright (C) 2013 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@codablock
Copy link
Author

Setting up gitian now, because even if I install gcc-4.8 locally it's still not reproducable

@codablock codablock force-pushed the backport_bitcoin_perutxo branch from 83945a6 to 7e3e9e4 Compare October 30, 2017 11:59
@codablock
Copy link
Author

codablock commented Oct 30, 2017

Ok, builds are passing now. It was not a difference in gcc version, but a difference in boost versions and the use of boost::unordered_map vs. std::unordered_map. At the same time, I skipped backporting bitcoin#10249 as I planned to have C++11 related changes as part of my other backportings. I added this PR now, which changes boost::unordered_map to std::unordered_map, which also fixes the issue I had with compilation.

@OlegGirko
Copy link

OlegGirko commented Oct 30, 2017

I've spent some time yesterday trying to find the cause of this problem, but I was too late to report my findings.

The problem is indeed with using emplace() method of boost::unordered_map with 3 arguments std::piecewise_construct, tuple of arguments for key, tuple of arguments for value.
This is a simplified test case to check for this problem:

#include <utility>
#include <tuple>
#include <boost/unordered_map.hpp>

class C {
    int a_;
public:
    explicit C(int &&a): a_(std::move(a)) {}
};

int main() {
    boost::unordered_map<int, C> c_map;
    c_map.emplace(std::piecewise_construct,
                  std::forward_as_tuple(1),
                  std::forward_as_tuple(2));
    return 0;
}

Compilation fails in both Ubuntu with GCC 4.8.4 and Fedora with GCC 7.2.1.
Changing boost::unordered_map to std::unordered_map in example above fixes the problem.

Deep inside implementation of boost::unordered_map rvalue reference is lost when using std::get()` function template to retrieve tuple element. Here is a test case:

include <utility>
#include <tuple>

class C {
    int a_;
public:
    explicit C(int &&a): a_(std::move(a)) {}
};

int main() {
    std::tuple<int&&, int&&> t = std::forward_as_tuple(1, 2);
    C c(std::get<0>(t));
    return 0;
}

It's interesting to note that std::unordered_map uses std::get() as well, but in combination with std::forward(), so it doesn't lose rvalue reference. If you wrap std::get<0>(t) inside std::forward<int&&>() in the example above, compilation problem is fixed.

@OlegGirko
Copy link

Although this particular problem is solved by using std::unordered_map instead of boost::unordered_map, I decided to check whether the problem with boost::unordered_map still exists in Boost and found that it was fixed in Boost 1.65: it has emplace() method that forwards arguments to std::pair's constructor, and leaves special handling of std::piecewise_construct only for the case when two remaining arguments have boost::tuple type.

It's interesting to note that there is also no problem with emplacing using std::piecewise_construct in earlier versions of Boost (tested with Boost 1.54). This is because earlier versions have no special handling for std::piecewise_construct yet, so they just pass 3 arguments (std::piecewise_construct and 2 tuples) straight to std::pair's constructor that is already C++11-compatible and handles these arguments correctly.

@codablock codablock force-pushed the backport_bitcoin_perutxo branch from 7e3e9e4 to bff8b16 Compare October 31, 2017 07:30
@codablock
Copy link
Author

Now Travis isn't even starting the build with error "An error occurred while generating the build script."
@schinzelh Can you check whats wrong?

@schinzelh
Copy link

@codablock Travis is down —> https://www.traviscistatus.com

@codablock
Copy link
Author

Ah ok, next time I'll check that by myself :D

About the failing tests I had after fixing the build issue: These were caused by an erroneous merge resolution I did in dash-tx.cpp. I removed too much code which was surrounded by SegWit changes. This is fixed now and will hopefully give green builds after Travis is up again.

@codablock codablock force-pushed the backport_bitcoin_perutxo branch 5 times, most recently from 444f6b1 to 528d1ec Compare October 31, 2017 20:12
laanwj and others added 10 commits October 31, 2017 21:19
f478d98 Fix some empty vector references (Pieter Wuille)

Tree-SHA512: a22022b9060cd39f8d349e8dd24490614c0028eae2fbc7186d0d26b1d47529049b2daee41f093c0722130274a0b6f7f8671c2295c8cb4a97028771eaff080734
e6756ad Switch CCoinsMap from boost to std unordered_map (Pieter Wuille)
344a2c4 Add support for std::unordered_{map,set} to memusage.h (Pieter Wuille)

Tree-SHA512: 51288301e7c0f29ffac8c59f4cc73ddc36b7abeb764009da6543f2eaeeb9f89bd47dde48131a7e0aefad8f7cb0b74b2f33b8be052c8e8a718339c3e6bb963447
5898279 scripted-diff: various renames for per-utxo consistency (Pieter Wuille)
a5e02bc Increase travis unit test timeout (Pieter Wuille)
73de2c1 Rename CCoinsCacheEntry::coins to coin (Pieter Wuille)
119e552 Merge CCoinsViewCache's GetOutputFor and AccessCoin (Pieter Wuille)
580b023 [MOVEONLY] Move old CCoins class to txdb.cpp (Pieter Wuille)
8b25d2c Upgrade from per-tx database to per-txout (Pieter Wuille)
b2af357 Reduce reserved memory space for flushing (Pieter Wuille)
41aa5b7 Pack Coin more tightly (Pieter Wuille)
97072d6 Remove unused CCoins methods (Pieter Wuille)
ce23efa Extend coins_tests (Pieter Wuille)
5083079 Switch CCoinsView and chainstate db from per-txid to per-txout (Pieter Wuille)
4ec0d9e Refactor GetUTXOStats in preparation for per-COutPoint iteration (Pieter Wuille)
13870b5 Replace CCoins-based CTxMemPool::pruneSpent with isSpent (Pieter Wuille)
05293f3 Remove ModifyCoins/ModifyNewCoins (Pieter Wuille)
961e483 Switch tests from ModifyCoins to AddCoin/SpendCoin (Pieter Wuille)
8b3868c Switch CScriptCheck to use Coin instead of CCoins (Pieter Wuille)
c87b957 Only pass things committed to by tx's witness hash to CScriptCheck (Matt Corallo)
f68cdfe Switch from per-tx to per-txout CCoinsViewCache methods in some places (Pieter Wuille)
0003911 Introduce new per-txout CCoinsViewCache functions (Pieter Wuille)
bd83111 Optimization: Coin&& to ApplyTxInUndo (Pieter Wuille)
cb2c7fd Replace CTxInUndo with Coin (Pieter Wuille)
422634e Introduce Coin, a single unspent output (Pieter Wuille)
7d991b5 Store/allow tx metadata in all undo records (Pieter Wuille)
c3aa0c1 Report on-disk size in gettxoutsetinfo (Pieter Wuille)
d342424 Remove/ignore tx version in utxo and undo (Pieter Wuille)
7e00322 Add specialization of SipHash for 256 + 32 bit data (Pieter Wuille)
e484652 Introduce CHashVerifier to hash read data (Pieter Wuille)
f54580e error() in disconnect for disk corruption, not inconsistency (Pieter Wuille)
e66dbde Add SizeEstimate to CDBBatch (Pieter Wuille)

Tree-SHA512: ce1fb1e40c77d38915cd02189fab7a8b125c7f44d425c85579d872c3bede3a437760997907c99d7b3017ced1c2de54b2ac7223d99d83a6658fe5ef61edef1de3
…rsor()

3ff1fa8 Use override keyword on CCoinsView overrides (Russell Yanofsky)
24e44c3 Don't return stale data from CCoinsViewCache::Cursor() (Russell Yanofsky)

Tree-SHA512: 08699dae0925ffb9c018f02612ac6b7eaf73ec331e2f4f934f1fe25a2ce120735fa38596926e924897c203f7470e99f0a99cf70d2ce31ff428b105e16583a861
…tweak

9417d7a Be much more agressive in AccessCoin docs. (Matt Corallo)
f58349c Restore some assert semantics in sigop cost calculations (Matt Corallo)
3533fb4 Return a bool in SpendCoin to restore pre-per-utxo assert semantics (Matt Corallo)
ec1271f Remove useless mapNextTx lookup in CTxMemPool::TrimToSize. (Matt Corallo)

Tree-SHA512: 158a4bce063eac93e1d50709500a10a7cb1fb3271f10ed445d701852fce713e2bf0da3456088e530ab005f194ef4a2adf0c7cb23226b160cecb37a79561f29ca
…eCoin

5257698 Change semantics of HaveCoinInCache to match HaveCoin (Alex Morcos)

Tree-SHA512: 397e9ba28646b81fffa53e55064735d4d242aaffdf8484506825f785b0e414f334e4c5cd1e4e1dd9a4b6d1f6954c7ecad15429934a1c4e8d39f596cbd9f5dd80
21180ff Simplify return values of GetCoin/HaveCoin(InCache) (Pieter Wuille)

Tree-SHA512: eae0aa64fa1308191100cdc7cdc790c825f33b066c200a18b5895d7d5806cee1cc4caba1766ef3379a7cf93dde4bbae2bc9be92947935f5741f5c126d3ee991b
sipa and others added 5 commits October 31, 2017 21:19
21d4afa Comment clarifications in coins.cpp (Alex Morcos)
3c8a9ae Add belt-and-suspenders in DisconnectBlock (Alex Morcos)

Tree-SHA512: d83e12ed71674faaaaebc03ffa1e2276984c35a29db419268ac9e14a45b33ccab716e3606dff8cfe1dcee4bec6e4794d2ca90341f10d5684be80e3fee61addf8
…n keypress 'q'

542ce6e Report [CANCELLED] instead of [DONE] when shut down during txdb upgrade (Jonas Schnelli)
83fbea3 Report txdb upgrade not more often then every 10% (Jonas Schnelli)
06c5b6e Show txdb upgrade progress in debug log (Jonas Schnelli)
316fcb5 Allow to cancel the txdb upgrade via splashscreen callback (Jonas Schnelli)
ae09d45 Allow to shut down during txdb upgrade (Jonas Schnelli)
00cb69b [Qt] allow to execute a callback during splashscreen progress (Jonas Schnelli)

Tree-SHA512: 23190f23f441bfd60821e49f8b3698a6bef97eb0e0ee659328e4a7395769ecd1616420eacc38aa1fa0ff62b9de5f13a0098dc798cdec6bff649575cefebc0db2
efeb273 Force on-the-fly compaction during pertxout upgrade (Pieter Wuille)

Pull request description:

  It seems that LevelDB tends to leave the old "per txid" UTXO entries in the database lying around for a significant amount of time during and after the per-txout upgrade. This introduces a `CompactRange` function in the database wrapper, and invokes it after every batch of updates in `CCoinsViewDB::Upgrade()`. This lowers temporary disk usage during and after the upgrade.

Tree-SHA512: fbf964c0a33f4e73709c999c8a2bfdef974779c15820907398a2f8828f5fa3e4e153ddd9031d6fc5083be81e22b999b9bd826fd063ad8b88f55c5e8342503290
…B compactions

8842d1a Add undocumented -forcecompactdb to force LevelDB compactions (Pieter Wuille)

Pull request description:

Tree-SHA512: de91f3f574f75248fa6e5091089c840957fae5a972ebcd2b89493f7d777d4658560a6f5a3b43ab0c9b2c333ad98f9f185ae224c9caffc1a5e8df369cc414f123
861f9a2 Skip remainder of init if upgrade is cancelled (Matt Corallo)

Pull request description:

  Based on bitcoin#10919.
  Without this, if you cancel upgrade, you get a needless error:
  ERROR: VerifyDB(): *** irrecoverable inconsistency in block data at

Tree-SHA512: aa47665682c6605ada376f1c100ce17cf8c4312427929eb2e75306f2199b47cbcdb4e0d98d5efcfefff03947b2c0fcbd3aab487a4ed14d50607df685c91a03d0
@codablock codablock force-pushed the backport_bitcoin_perutxo branch from 528d1ec to 9a450f7 Compare October 31, 2017 20:19
@codablock
Copy link
Author

Builds are finally green. After fixing the previous test failures, I got the next one which only happened in Travis but not locally. "make check" was failing with out-of-bounds access to a vector, without telling me which one it was. After some Travis debugging with some log outputs and increased BOOST_TEST_LOG_LEVEL, it turned out that some stream read with size 0 was performed on an empty vector, which is fine normally, but crashes with stl debugging enabled. Adding bitcoin#10250 to this PR fixed the issue.

I'm repushing a clean version now (omitting the debug code), after this gives green Travis builds, we can finally merge.

@codablock codablock force-pushed the backport_bitcoin_perutxo branch from 9a450f7 to 4cd1991 Compare October 31, 2017 21:14
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

Nice job, thanks! 👍

utACK

@UdjinM6 UdjinM6 merged commit 83ec12f into dashpay:develop Nov 1, 2017
@codablock codablock deleted the backport_bitcoin_perutxo branch September 14, 2018 12:48
@codablock codablock mentioned this pull request May 27, 2019
PastaPastaPasta added a commit that referenced this pull request Jul 14, 2025
, bitcoin#25005, bitcoin#25410, bitcoin#24236, bitcoin#25438, bitcoin#25036, bitcoin#25489, bitcoin#25544, bitcoin#25594, bitcoin#13226, partial bitcoin#25218, bitcoin#26005 (wallet backports: part 6)

175970e merge bitcoin#13226: Optimize SelectCoinsBnB by tracking the selection by index rather than by position (Kittywhiskers Van Gogh)
27933a7 merge bitcoin#25594: Return BResult from restoreWallet (Kittywhiskers Van Gogh)
b75f1ad partial bitcoin#26005: Fix error handling (copy_file failure in RestoreWallet, and in general via interfaces) (Kittywhiskers Van Gogh)
9f5845c partial bitcoin#25218: introduce generic 'Result' class and connect it to CreateTransaction and GetNewDestination (Kittywhiskers Van Gogh)
db7e174 merge bitcoin#25544: don't iter twice when getting the cached debit/credit amount (Kittywhiskers Van Gogh)
48f5c10 merge bitcoin#25489: change `ScanForWalletTransactions` to use `Ticks(Dur2 d)` (Kittywhiskers Van Gogh)
4aafd98 merge bitcoin#25036: Save wallet scan progress (Kittywhiskers Van Gogh)
e2f053a merge bitcoin#25438: remove unused methods in classes `CDBIterator,CDBWrapper,CCoinsViewDBCursor` (Kittywhiskers Van Gogh)
79fcd30 merge bitcoin#24236: Remove utxo db upgrade code (Kittywhiskers Van Gogh)
d13ff52 fix: use URL quoting for password fields due to Base64 in pre-v0.12.2.x (Kittywhiskers Van Gogh)
7b6af94 fix: allow argument-free RPC calls with nodes running v0.12.2.x and lower (Kittywhiskers Van Gogh)
1026f11 fix: make `get_previous_release.py` translate triples for pre-v0.12.2.x (Kittywhiskers Van Gogh)
a0b2c7d fix: make `get_previous_releases.py` understand windows target triples (Kittywhiskers Van Gogh)
51237c9 merge bitcoin#25410: fix warning: "argument name 'feerate' in comment does not match parameter name" (Kittywhiskers Van Gogh)
2438b9f merge bitcoin#25005: remove extra wtx lookup in 'AvailableCoins' + several code cleanups (Kittywhiskers Van Gogh)
a498336 refactor: use initializer to set `nCoinType` instead of manual assignment (Kittywhiskers Van Gogh)
ae24276 merge bitcoin#25083: Set effective_value when initializing a COutput (Kittywhiskers Van Gogh)
a883a8e merge bitcoin#24666: Fix coinselection.h include, Make COutput a struct (Kittywhiskers Van Gogh)
f447b5d merge bitcoin#24711: Postpone wallet loading notification for encrypted wallets (Kittywhiskers Van Gogh)
39557f9 fix: include `BOOST_AUTO_TEST_SUITE_END` within namespace `wallet` (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * When backporting [bitcoin#23497](bitcoin#23497) (see [dash#6732](#6732)), some `BOOST_AUTO_TEST_SUITE_END()`s were placed _outside_ the `wallet` namespace scope, which resulted in incorrect behavior when rebasing this pull request over [dash#6732](#6732). This has been resolved.

  * Backports of [bitcoin#25083](bitcoin#25083) and [bitcoin#25005](bitcoin#25005) do not include changes to `wallet/rpc/spend.cpp` as the `sendall()` RPC has not been implemented in `develop` (f453998) as of this writing.

  * Backporting [bitcoin#24236](bitcoin#24236) required a fair number of additional considerations, firstly because the pull request introduces a test, `feature_unsupported_utxo_db.py`, that requires the last-capable of version Dash Core to generate a legacy UTXO database (the current database format debuted in [dash#1701](#1701), included in v0.12.2.2) and secondly, because v0.15 is the earliest expected version of Dash Core to work smoothly with our test framework ([source](https://github.com/dashpay/dash/blob/f453998bb76dee8fe016138da664dc281fc7f1d3/test/functional/mempool_compatibility.py#L29)).

    As we require the last release cycle that generates the legacy UTXO database by default, we used the last version of the v0.12.1.x family, v0.12.1.5 as the old node used in `feature_unsupported_utxo_db.py`. This brings unique challenges which are elaborated on below.

    * The test framework relies on `generatetoaddress`, a wallet-independent RPC call that was introduced in [bitcoin#7671](bitcoin#7671) to replace the old wallet-dependent `generate` RPC. It was backported as 760d58e in [dash#1790](#1790) and debuted in v0.12.3. We cannot use the `self.generate()` helper as simply dispatches a `generatetoaddress` ([source](https://github.com/dashpay/dash/blob/f453998bb76dee8fe016138da664dc281fc7f1d3/test/functional/test_framework/test_node.py#L343-L345)), which does not exist in v0.12.1.5.

      We work around this by using the more archaic `self.nodes[0].rpc.generate()`, in line with usage in `rpc_generate.py` ([source](https://github.com/dashpay/dash/blob/f453998bb76dee8fe016138da664dc281fc7f1d3/test/functional/rpc_generate.py#L25-L26)).

    * Releases before v0.12.3.x did not include the target triples in their naming convention (see tag [`v0.12.3.1`](https://github.com/dashpay/dash/releases/tag/v0.12.3.1) vs [`v0.12.1.5`](https://github.com/dashpay/dash/releases/tag/v0.12.1.5)), which causes problems when fetching packages based on the target `HOST` of the build environment. This has been remedied by introducing awareness for `RPi2`, `osx` and `linux{32,64}` labels.

      * Note, `RPi2` corresponds to `arm-linux-gnueabihf` based on the Gitian descriptor that was removed in [dash#2183](#2183) ([source](https://github.com/dashpay/dash/blob/dac090964fbdd96d896f725932aada71db736abf/contrib/gitian-descriptors/gitian-rpi2.yml#L26))

     * This was also taken as an opportunity to introduce awareness for the `win32` and `win64` labels which persist even now (see tag [`v22.1.2`](https://github.com/dashpay/dash/releases/tag/v22.1.2))

    * Currently, when we want to make RPC calls without arguments, a blank object is sent to by the RPC dispatcher. This is acceptable as objects are supported since [bitcoin#8811](bitcoin#8811) (backported as eb7a6b0 in [dash#1851](#1858)), included in v0.12.3.

      Unfortunately, since we are dealing with a version of Dash that is ancient even by previous release standards, this prevents the dispatcher from even acknowledging the RPC server is active (see below).

      <details>

      <summary>Test failure:</summary>

      ```
      $ ./test/functional/feature_unsupported_utxo_db.py
      2025-06-24T15:06:53.642000Z TestFramework (INFO): PRNG seed is: 1995903613070947610
      2025-06-24T15:06:53.642000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_1tqzf43o
      2025-06-24T15:06:53.642000Z TestFramework (INFO): Create previous version (v0.12.1.5) utxo db
      2025-06-24T15:06:53.895000Z TestFramework (ERROR): JSONRPC error
      Traceback (most recent call last):
        File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/test_framework.py", line 161, in main
          self.run_test()
        File "/home/eclair/Repositories/dashpay/dash/./test/functional/feature_unsupported_utxo_db.py", line 35, in run_test
          self.start_node(0)
        File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/test_framework.py", line 647, in start_node
          node.wait_for_rpc_connection()
        File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/test_node.py", line 276, in wait_for_rpc_connection
          rpc.getblockcount()
        File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/coverage.py", line 49, in __call__
          return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/authproxy.py", line 144, in __call__
          raise JSONRPCException(response['error'], status)
      test_framework.authproxy.JSONRPCException: Params must be an array (-32600)
      2025-06-24T15:06:53.947000Z TestFramework (INFO): Stopping nodes
      [...]
      ```

      </details>

        * This is worked around by dispatching an empty array instead of an empty object, it has the same practical effect (conveying the lack of arguments) and is forwards-compatible.

    * Currently, the password used for RPC authentication in Base16 encoded. This is a result of [dash#1454](#1454), which switched away from Base64. Unfortunately, this benefit only extends to v0.12.2.0 onwards, which is too new for the old node needed in our functional test.

      Worse still, it was not the URL-safe variant of Base64, so attempting to establish a connection with the old node resulted in sporadic failures (see below).

      <details>

      <summary>Test failure:</summary>

      ```
      $ ./test/functional/feature_unsupported_utxo_db.py
      2025-06-24T15:13:27.517000Z TestFramework (INFO): PRNG seed is: 3228606794541395700
      2025-06-24T15:13:27.518000Z TestFramework (INFO): Initializing test directory /tmp/dash_func_test_97d987gf
      2025-06-24T15:13:27.518000Z TestFramework (INFO): Create previous version (v0.12.1.5) utxo db
      2025-06-24T15:13:27.769000Z TestFramework (ERROR): Unexpected exception caught during testing
      Traceback (most recent call last):
        File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/test_framework.py", line 161, in main
          self.run_test()
        File "/home/eclair/Repositories/dashpay/dash/./test/functional/feature_unsupported_utxo_db.py", line 35, in run_test
          self.start_node(0)
        File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/test_framework.py", line 647, in start_node
          node.wait_for_rpc_connection()
        File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/test_node.py", line 270, in wait_for_rpc_connection
          rpc = get_rpc_proxy(
                ^^^^^^^^^^^^^^
        File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/util.py", line 341, in get_rpc_proxy
          proxy = AuthServiceProxy(url, **proxy_kwargs)
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        File "/home/eclair/Repositories/dashpay/dash/test/functional/test_framework/authproxy.py", line 78, in __init__
          authpair = user + b':' + passwd
                    ~~~~~^~~~~~
      TypeError: unsupported operand type(s) for +: 'NoneType' and 'bytes'
      2025-06-24T15:13:27.822000Z TestFramework (INFO): Stopping nodes
      [...]
      ```

      </details>

        * This is worked around by escaping URL special characters for the password field. It is practically a no-op for the Base16 passwords generated from v0.12.2.0 onwards but allows connection and authentication with older versions of Dash Core.

  ## How Has This Been Tested?

  A full CoinJoin session run on 89cd497

  ![CoinJoin session run on build 89cd497](https://github.com/user-attachments/assets/29caa752-0103-490b-a0e1-56c1030105f2)

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 175970e
  PastaPastaPasta:
    utACK 175970e

Tree-SHA512: 6a6fe593e25cc5c99da41333a334655010cc387f8c3e390d472de29782aaa6aa67276e2e00a90609facabe4f1817fa0152e6b6765f97e07541fd378ef0f4d566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants