b56607a89ba112083f2b0a7b64ab18d66b26e2be Remove CCoinsViewCache::GetValueIn(...) (practicalswift)
Pull request description:
Remove `CCoinsViewCache::GetValueIn(...)`.
Fixes#18858.
It seems like `GetValueIn` was added in #748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in #8498 ("Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...", merged in 2017).
`CCoinsViewCache::GetValueIn(…)` performs money summation like this:
```c++
CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const
{
if (tx.IsCoinBase())
return 0;
CAmount nResult = 0;
for (unsigned int i = 0; i < tx.vin.size(); i++)
nResult += AccessCoin(tx.vin[i].prevout).out.nValue;
return nResult;
}
```
Note that no check is done to make sure that the resulting `nResult` is such that it stays within the money bounds (`MoneyRange(nResult)`), or that the summation does not trigger a signed integer overflow.
Proof of concept output:
```
coins.cpp:243:17: runtime error: signed integer overflow: 9223200000000000000 + 2100000000000000 cannot be represented in type 'long'
GetValueIn = -9221444073709551616
```
Proof of concept code:
```c++
CMutableTransaction mutable_transaction;
mutable_transaction.vin.resize(4393);
Coin coin;
coin.out.nValue = MAX_MONEY;
assert(MoneyRange(coin.out.nValue));
CCoinsCacheEntry coins_cache_entry;
coins_cache_entry.coin = coin;
coins_cache_entry.flags = CCoinsCacheEntry::DIRTY;
CCoinsView backend_coins_view;
CCoinsViewCache coins_view_cache{&backend_coins_view};
CCoinsMap coins_map;
coins_map.emplace(COutPoint{}, std::move(coins_cache_entry));
coins_view_cache.BatchWrite(coins_map, {});
const CAmount total_value_in = coins_view_cache.GetValueIn(CTransaction{mutable_transaction});
std::cout << "GetValueIn = " << total_value_in << std::endl;
```
ACKs for top commit:
MarcoFalke:
ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be
promag:
Code review ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be.
jb55:
ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be
hebasto:
ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.
Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6
d9d79576f423cd9c5cef4547c7e3648dbb339460 Preserve a format of RPC command definitions (Kostiantyn Stepaniuk)
Pull request description:
Currently, RPC commands are formatted in a way that it's easy to read
and that `test/lint/check-rpc-mappings.py` can parse it.
To void breaking `test/lint/check-rpc-mappings.py` script by running
`clang-format`, RPC command definitions should be disabled for clang-format.
Tree-SHA512: e17d20ec0e6c4e19410198b55687ebbe6fa01654d214d4578cd16c00b872bf8b0b306594a45523685cd2e9d9280702e00471d9366e87954428e8bbeacd8cad60
21fa0a44abe8c1b5c452e097eab20cf0ae988805 [docs] use consistent naming for possible_overwrite (John Newbery)
2685c214cce4b07695273503e60350e3f05fe3e2 [tests] small whitespace fixup (John Newbery)
e9936966c08bd8a6ac02828131f619ddaa1ced13 scripted-diff: Rename PRUNED to SPENT in coins tests (John Newbery)
c205979031ff4e8e32a5f05bae813405f233fccd [docs] Improve commenting in coins.cpp|h (John Newbery)
Pull request description:
- Add full commenting for spentness / DIRTYness / FRESHness and which combinations are valid
- Remove the 'pruned' terminology, which doesn't make sense since per-txout chainstate db was merged (#10195).
- Rename `potential_overwrite` to `possible_overwrite` to standardize terminology (there were previously examples of both, which made searching the codebase difficult).
- Make other minor improvements to the comments
ACKs for top commit:
jonatack:
Re-ACK 21fa0a4 per `git diff 98bee55 21fa0a4` the only change since my previous review is the following code commenting diff in `src/coins.cpp::L177-179`; rebuilt/ran unit tests anyway as a sanity check on the unit test changes.
Tree-SHA512: 391e01588ef5edb417250080cec17361f982c4454bc5f8c6d78bbd528c68a2bb94373297760691295c24660ce1022ad3ef7599762f736c8eed772ce096d38c3d
1404c574037da331c233317618b9b90d3329ed33 [doc] Coin: explain that IsSpent() can also mean never existed (Sjors Provoost)
Pull request description:
This can be especially confusing where `AccessCoin()` is used with logic like this:
```c++
while (iter.n < MAX_OUTPUTS_PER_BLOCK) {
const Coin& alternate = view.AccessCoin(iter);
if (!alternate.IsSpent()) return alternate;
```
ACKs for top commit:
practicalswift:
ACK 1404c574037da331c233317618b9b90d3329ed33
MarcoFalke:
ACK 1404c574037da331c233317618b9b90d3329ed33
jnewbery:
utACK 1404c574037da331c233317618b9b90d3329ed33
Tree-SHA512: 418618dd7e08bd5cc8360e3501d0f57e34100e5101ad3b8e0a819923fa860f44c7f2fada0f8447a1af3c2601fd72bfe619b91ff2f26f7133ceaeb0c98b017b12
e980214bc4fd49530e8d50fe0a6657b8583bc6b5 serialization: prevent int overflow for big Coin::nHeight (pierrenn)
Pull request description:
This is an attempt to fix fuzzer issues 1,2,8 reported by practicalswift here : https://github.com/bitcoin/bitcoin/issues/18046
The fuzzer harness doesn't prevent deserialization of unrealistic high values for `Coin::nHeight`. In the [provided examples](https://github.com/bitcoin/bitcoin/issues/18046), we have :
- `blockundo_deserialize` : the varint `0x8DD88DD700` is deserialized as `3944983552` in `Coin::nHeight` (`TxInOutFormatter::Unser`)
- `coins_deserialize` : the varint `0x8DD5D5EC40` is deserialized as `3939874496` similarly
- `txundo_deserialize`: the varint `0x8DCD828F01` is deserialized as `3921725441` in `Coin::nHeight` (`Coin::Unserialize`)
Since `Coin::nHeight` is 31 bit long, multiplying a large value by 2 triggers the fuzzer.
AFAIK those values are unrealistic (~70k years for the smallest..). I've looked a bit a reducing the range of values the fuzzer can deserialize, but this seems to be too much code change for not much.
Hence this PR chooses to static cast `nHeight` when re-serializing; it seems to be the less intrusive/safest way to prevent the fuzzer output.
Another more "upstream" approach would be to limit `Coin::nHeight` values to something more realistic, e.g. `0xFFFFFFF` (~5k years) :
de3a30bab2/src/undo.h (L39) and de3a30bab2/src/coins.h (L71)
Thanks !
NB: i was also not sure about the component/area to prefix the PR/commit with.. ?
ACKs for top commit:
practicalswift:
ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5 -- patch looks correct
promag:
ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5.
sipa:
utACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5
MarcoFalke:
re-ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5 🎑
ryanofsky:
Code review ACK e980214bc4fd49530e8d50fe0a6657b8583bc6b5. Just removed ternary ? 1 : 0 and replaced / 2 with >> 1 since last review
Tree-SHA512: 905fc9e5e52a6857abee4a1c863751767835965804bb8c39474f27a120f65399ff4ba7a49ef1da0ba565379f8c12095bd384b6c492cf06776f01b2db68d522b8
fa09110ebb5e485b17a767fca198819fcbe7c16e doc: Fix typo in Coin doxygen comment (MarcoFalke)
Pull request description:
`CTxOutCompressor` has been renamed in commit 4de934b9b5b4be1bac8fe205f4ee9a79e772dc34, so rename it in the docs as well.
ACKs for top commit:
laanwj:
ACK fa09110ebb5e485b17a767fca198819fcbe7c16e
hebasto:
ACK fa09110ebb5e485b17a767fca198819fcbe7c16e
Tree-SHA512: e16a21ac3112a67ee7d5ffabb3f47103aed8f91fdebf1bf96311cd0b7bdb9b7323ed826bfa95517386d4128ff0ae2c7c13bad047a7c5a0cc2458be7a43119157
67d99900b0d770038c9c5708553143137b124a6c make SaltedOutpointHasher noexcept (Martin Ankerl)
Pull request description:
If the hash is not `noexcept`, `unorderd_map` has to assume that it can throw an exception. Thus when rehashing care needs to be taken. libstdc++ solves this by simply caching the hash value, which increases memory of each node by 8 bytes. Adding `noexcept` prevents this caching. In my experiments with `-reindex-chainstate -stopatheight=594000`, memory usage (maximum resident set size) has decreased by 9.4% while runtime has increased by 1.6% due to additional hashing. Additionally, memusage::DynamicUsage() is now more accurate and does not underestimate.
| | runtime h:mm:ss | max RSS kbyte |
|---------------------------------------|-----------------|--------------|
| master | 4:13:59 | 7696728 |
| 2019-09-SaltedOutpointHasher-noexcept | 4:18:11 | 6971412 |
| change | +1.65% | -9,42% |
Comparison of progress masters vs. 2019-09-SaltedOutpointHasher-noexcept
ACKs for top commit:
jamesob:
Tested ACK 67d99900b0
Tree-SHA512: 9c44e3cca993b5a564dd61ebd2926b9c4a238609ea4d283514c018236f977d935e35a384dd4696486fd3d78781dd2ba190bb72596e20a5e931042fa465872a0b
172f5fa738 Support deserializing into temporaries (Pieter Wuille)
2761bca997 Merge READWRITEMANY into READWRITE (Pieter Wuille)
Pull request description:
This is another fragment of improvements from #10785.
The current serialization code does not support serializing/deserializing from/to temporaries (like `s >> CFlatData(script)`). As a result, there are many invocations of the `REF` macro which in addition to changing the reference type also changes the constness. This is unnecessary in C++11 as we can use rvalue references now instead.
The first commit is an extra simplification we can make that removes the duplication of code between `READWRITE` and `READWRITEMANY` (and related functions).
Tree-SHA512: babfa9cb268cc3bc39917e4f0a90e4651c33d85032161e16547a07f3b257b7ca7940e0cbfd69f09439d26fafbb1a6cf6359101043407e2c7aeececf7f20b6eed
* scripted-diff: Replace #include "" with #include <> (ryanofsky)
-BEGIN VERIFY SCRIPT-
for f in \
src/*.cpp \
src/*.h \
src/bench/*.cpp \
src/bench/*.h \
src/compat/*.cpp \
src/compat/*.h \
src/consensus/*.cpp \
src/consensus/*.h \
src/crypto/*.cpp \
src/crypto/*.h \
src/crypto/ctaes/*.h \
src/policy/*.cpp \
src/policy/*.h \
src/primitives/*.cpp \
src/primitives/*.h \
src/qt/*.cpp \
src/qt/*.h \
src/qt/test/*.cpp \
src/qt/test/*.h \
src/rpc/*.cpp \
src/rpc/*.h \
src/script/*.cpp \
src/script/*.h \
src/support/*.cpp \
src/support/*.h \
src/support/allocators/*.h \
src/test/*.cpp \
src/test/*.h \
src/wallet/*.cpp \
src/wallet/*.h \
src/wallet/test/*.cpp \
src/wallet/test/*.h \
src/zmq/*.cpp \
src/zmq/*.h
do
base=${f%/*}/ relbase=${base#src/} sed -i "s:#include \"\(.*\)\"\(.*\):if test -e \$base'\\1'; then echo \"#include <\"\$relbase\"\\1>\\2\"; else echo \"#include <\\1>\\2\"; fi:e" $f
done
-END VERIFY SCRIPT-
Signed-off-by: Pasta <pasta@dashboost.org>
* scripted-diff: Replace #include "" with #include <> (Dash Specific)
-BEGIN VERIFY SCRIPT-
for f in \
src/bls/*.cpp \
src/bls/*.h \
src/evo/*.cpp \
src/evo/*.h \
src/governance/*.cpp \
src/governance/*.h \
src/llmq/*.cpp \
src/llmq/*.h \
src/masternode/*.cpp \
src/masternode/*.h \
src/privatesend/*.cpp \
src/privatesend/*.h
do
base=${f%/*}/ relbase=${base#src/} sed -i "s:#include \"\(.*\)\"\(.*\):if test -e \$base'\\1'; then echo \"#include <\"\$relbase\"\\1>\\2\"; else echo \"#include <\\1>\\2\"; fi:e" $f
done
-END VERIFY SCRIPT-
Signed-off-by: Pasta <pasta@dashboost.org>
* build: Remove -I for everything but project root
Remove -I from build system for everything but the project root,
and built-in dependencies.
Signed-off-by: Pasta <pasta@dashboost.org>
# Conflicts:
# src/Makefile.test.include
* qt: refactor: Use absolute include paths in .ui files
* qt: refactor: Changes to make include paths absolute
This makes all include paths in the GUI absolute.
Many changes are involved as every single source file in
src/qt/ assumes to be able to use relative includes.
Signed-off-by: Pasta <pasta@dashboost.org>
# Conflicts:
# src/qt/dash.cpp
# src/qt/optionsmodel.cpp
# src/qt/test/rpcnestedtests.cpp
* test: refactor: Use absolute include paths for test data files
* Recommend #include<> syntax in developer notes
* refactor: Include obj/build.h instead of build.h
* END BACKPORT #11651 Remove trailing whitespace causing travis failure
* fix backport 11651
Signed-off-by: Pasta <pasta@dashboost.org>
* More of 11651
* fix blockchain.cpp
Signed-off-by: pasta <pasta@dashboost.org>
* Add missing "qt/" in includes
* Add missing "test/" in includes
* Fix trailing whitespaces
Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: MeshCollider <dobsonsa68@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
2a07f878a Refactor: Modernize disallowed copy constructors/assignment (Dan Raviv)
Pull request description:
Use C++11's better capability of expressing an interface of a non-copyable class by publicly deleting its copy ctor and assignment operator instead of just declaring them private.
Tree-SHA512: 878f446be5a136bb2a90643aaeaca62948b575e6ef71ccc5b4b8f373e66f36ced00665128f36504e0ccfee639863d969329c4276154ef9f2a9de9137f0801e01
176c021 [qa] Test non-atomic chainstate writes (Suhas Daftuar)
d6af06d Dont create pcoinsTip until after ReplayBlocks. (Matt Corallo)
eaca1b7 Random db flush crash simulator (Pieter Wuille)
0580ee0 Adapt memory usage estimation for flushing (Pieter Wuille)
013a56a Non-atomic flushing using the blockchain as replay journal (Pieter Wuille)
b3a279c [MOVEONLY] Move LastCommonAncestor to chain (Pieter Wuille)
Tree-SHA512: 47ccc62303f9075c44d2a914be75bd6969ff881a857a2ff1227f05ec7def6f4c71c46680c5a28cb150c814999526797dc05cf2701fde1369c06169f46eccddee
1c897fc Missing includes (Jorge Timón)
a1fd450 Trivial: Remove unneeded includes from .h: (Jorge Timón)
Tree-SHA512: ada3e62cc2435e58172a88b380be371b717a05725956c15e5493b6e19fe2903e5e6e43fd22dc24699333a0e8a0c7b42eb1ae61b41cb4ba82495be18e2d4ef3c6
move "#include "chain.h"" down a line
Signed-off-by: Pasta <pasta@dashboost.org>
* Drop nBudgetProposalEstablishingTime
* Refactor: replace `== COutPoint()` with `.IsNull()`
* Refactor: add `operator bool()` to CMasternodePing
* Refactor: actually use `operator bool()` for CPendingDsaRequest
* Refactor: fixing code style in TrafficGraphData
* Fix some comments and whitespaces
* Drop CGovernanceVote::GetTypeHash()
* Drop legacy X11 code
No longer used... if it ever was used at all.
* Move `<boost/foreach.hpp>` out of coins.h
* Simplify CMasternodeBlockPayees::GetRequiredPaymentsString()
Also less of boost::lexical_cast
* Drop CTxDSIn::nSentTimes
* Fix few warnings
* fix warning (timer)
* fix nit
d59a518 Use fixed preallocation instead of costly GetSerializeSize (Pieter Wuille)
25a211a Add optimized CSizeComputer serializers (Pieter Wuille)
a2929a2 Make CSerAction's ForRead() constexpr (Pieter Wuille)
a603925 Avoid -Wshadow errors (Pieter Wuille)
5284721 Get rid of nType and nVersion (Pieter Wuille)
657e05a Make GetSerializeSize a wrapper on top of CSizeComputer (Pieter Wuille)
fad9b66 Make nType and nVersion private and sometimes const (Pieter Wuille)
c2c5d42 Make streams' read and write return void (Pieter Wuille)
50e8a9c Remove unused ReadVersion and WriteVersion (Pieter Wuille)
9417d7a33 Be much more agressive in AccessCoin docs. (Matt Corallo)
f58349ca8 Restore some assert semantics in sigop cost calculations (Matt Corallo)
3533fb4d3 Return a bool in SpendCoin to restore pre-per-utxo assert semantics (Matt Corallo)
ec1271f2b Remove useless mapNextTx lookup in CTxMemPool::TrimToSize. (Matt Corallo)
Tree-SHA512: 158a4bce063eac93e1d50709500a10a7cb1fb3271f10ed445d701852fce713e2bf0da3456088e530ab005f194ef4a2adf0c7cb23226b160cecb37a79561f29ca
3ff1fa8 Use override keyword on CCoinsView overrides (Russell Yanofsky)
24e44c3 Don't return stale data from CCoinsViewCache::Cursor() (Russell Yanofsky)
Tree-SHA512: 08699dae0925ffb9c018f02612ac6b7eaf73ec331e2f4f934f1fe25a2ce120735fa38596926e924897c203f7470e99f0a99cf70d2ce31ff428b105e16583a861
589827975 scripted-diff: various renames for per-utxo consistency (Pieter Wuille)
a5e02bc7f Increase travis unit test timeout (Pieter Wuille)
73de2c1ff Rename CCoinsCacheEntry::coins to coin (Pieter Wuille)
119e552f7 Merge CCoinsViewCache's GetOutputFor and AccessCoin (Pieter Wuille)
580b02309 [MOVEONLY] Move old CCoins class to txdb.cpp (Pieter Wuille)
8b25d2c0c Upgrade from per-tx database to per-txout (Pieter Wuille)
b2af357f3 Reduce reserved memory space for flushing (Pieter Wuille)
41aa5b79a Pack Coin more tightly (Pieter Wuille)
97072d668 Remove unused CCoins methods (Pieter Wuille)
ce23efaa5 Extend coins_tests (Pieter Wuille)
508307968 Switch CCoinsView and chainstate db from per-txid to per-txout (Pieter Wuille)
4ec0d9e79 Refactor GetUTXOStats in preparation for per-COutPoint iteration (Pieter Wuille)
13870b56f Replace CCoins-based CTxMemPool::pruneSpent with isSpent (Pieter Wuille)
05293f3cb Remove ModifyCoins/ModifyNewCoins (Pieter Wuille)
961e48397 Switch tests from ModifyCoins to AddCoin/SpendCoin (Pieter Wuille)
8b3868c1b Switch CScriptCheck to use Coin instead of CCoins (Pieter Wuille)
c87b957a3 Only pass things committed to by tx's witness hash to CScriptCheck (Matt Corallo)
f68cdfe92 Switch from per-tx to per-txout CCoinsViewCache methods in some places (Pieter Wuille)
000391132 Introduce new per-txout CCoinsViewCache functions (Pieter Wuille)
bd83111a0 Optimization: Coin&& to ApplyTxInUndo (Pieter Wuille)
cb2c7fdac Replace CTxInUndo with Coin (Pieter Wuille)
422634e2f Introduce Coin, a single unspent output (Pieter Wuille)
7d991b55d Store/allow tx metadata in all undo records (Pieter Wuille)
c3aa0c119 Report on-disk size in gettxoutsetinfo (Pieter Wuille)
d34242430 Remove/ignore tx version in utxo and undo (Pieter Wuille)
7e0032290 Add specialization of SipHash for 256 + 32 bit data (Pieter Wuille)
e484652fc Introduce CHashVerifier to hash read data (Pieter Wuille)
f54580e7e error() in disconnect for disk corruption, not inconsistency (Pieter Wuille)
e66dbde6d Add SizeEstimate to CDBBatch (Pieter Wuille)
Tree-SHA512: ce1fb1e40c77d38915cd02189fab7a8b125c7f44d425c85579d872c3bede3a437760997907c99d7b3017ced1c2de54b2ac7223d99d83a6658fe5ef61edef1de3
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
Compute the value of inputs that already are in the chain at time of mempool entry and only increase priority due to aging for those inputs. This effectively changes the CTxMemPoolEntry's GetPriority calculation from an upper bound to a lower bound.
The docs talk about bits 2 and 4 instead of 2 and 3, and bits are being
indexed from both 1 (describing nCode) and 0 (in second example).
Changed to use zero-indexing for all.
When processing a new transaction, in addition to spending the Coins of its txin's it creates a new Coins for its outputs. The existing ModifyCoins function will first make sure this Coins does not already exist. It can not exist due to BIP 30, but because of that the lookup can't be cached and always has to go to the database. Since we are creating the coins to match the new tx anyway, there is no point in checking if they exist first anyway. However this should not be used for coinbase tx's in order to preserve the historical behavior of overwriting the two existing duplicate tx pairs.
