Skip to content
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.

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

Backport bitcoin perutxo #1701

Merged
merged 15 commits into from
Nov 1, 2017
Merged

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
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
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
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
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.

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
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