Skip to content

Remove CCoinsViewCache::GetValueIn(...) #18859

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.

Sign up for GitHub

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

Jump to bottom
Merged
maflcko merged 1 commit into from
May 4, 2020
Merged

Remove CCoinsViewCache::GetValueIn(...) #18859

maflcko merged 1 commit into from
May 4, 2020

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented May 3, 2020

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:

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:

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;

@maflcko
Copy link
Member

maflcko commented May 3, 2020

Please don't remove the tests and benchmarks. Just because some imaginary input causes an integer overflow in a helper function doesn't mean all tests using it should be removed. You can inline the function where it is used of move it to the test library in src/test/util

@practicalswift
Copy link
Contributor Author

practicalswift commented May 3, 2020

Just because some imaginary input causes an integer overflow in a helper function […]

Leaving this one as up for grabs.

@sipa
Copy link
Member

sipa commented May 3, 2020

@MarcoFalke What is the point of keeping a function around that is not used outside of tests?

@maflcko
Copy link
Member

maflcko commented May 3, 2020

The function calls AccessCoin, which is used to benchmark coin access in the coins_caching benchmark.

See also the docstring // Microbenchmark for simple accesses to a CCoinsViewCache database

If the call to AccessCoin is removed, the you might as well remove the whole benchmark

@practicalswift
Copy link
Contributor Author

practicalswift commented May 3, 2020

@MarcoFalke Just so that I understand: would you be fine with anyone using CCoinsViewCache::GetValueIn(...) as it is currently formulated within Bitcoin Core?

@maflcko
Copy link
Member

maflcko commented May 3, 2020

Oh, wait the benchmark does the coins access via AreInputsStandard. My bad.

@maflcko maflcko reopened this May 3, 2020
@maflcko
Copy link
Member

maflcko commented May 3, 2020

ACK b56607a

@maflcko maflcko added this to the 0.21.0 milestone May 3, 2020
@promag
Copy link
Contributor

promag commented May 3, 2020

Code review ACK b56607a.

The removed CCoinsViewCache::GetValueIn is similar to AreInputsStandard and like @practicalswift mentions the summation is meh.

@DrahtBot
Copy link
Contributor

DrahtBot commented May 3, 2020

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto
hebasto approved these changes May 3, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK b56607a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

@jb55
Copy link
Contributor

jb55 commented May 4, 2020

ACK b56607a

@DrahtBot DrahtBot mentioned this pull request May 4, 2020
Closed
@maflcko
Copy link
Member

maflcko commented May 4, 2020

@practicalswift Sorry again that I misread the diff. Looks good to merge now, and thanks for removing the code.

@maflcko maflcko merged commit 74a1152 into bitcoin:master May 4, 2020
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 27, 2021
@practicalswift @PiRK
Remove CCoinsViewCache::GetValueIn(...)
96fc9f4 
Summary:
> Fixes #18858.
>
> It seems like `GetValueIn` was added in PR748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in [[bitcoin/bitcoin#8498 | PR8498]] ("Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...", merged in 2017).

This is a backport of Core [[bitcoin/bitcoin#18859 | PR18859]]

Test Plan: nja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9071
@practicalswift practicalswift deleted the GetValueIn branch April 10, 2021 19:41
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@hebasto hebasto hebasto approved these changes

Assignees

No one assigned

Labels

Tests UTXO Db and Indexes

Projects

None yet

Milestone

0.21.0

Development

Successfully merging this pull request may close these issues.

Signed integer overflow in CCoinsViewCache::GetValueIn(…)

7 participants

@practicalswift @maflcko @sipa @promag @DrahtBot @jb55 @hebasto