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.

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

Pay-to-script-hash (OP_EVAL replacement) #748

Merged
merged 5 commits into from
Jan 13, 2012
Merged

Pay-to-script-hash (OP_EVAL replacement) #748

merged 5 commits into from
Jan 13, 2012

Conversation

gavinandresen
Copy link
Contributor

This implements BIP 16, the replacement for pay-to-script hash.
https://en.bitcoin.it/wiki/BIP_0016

Tested mostly on testnet-in-a-box, but I did spend time today generating transactions on the testnet as a final sanity test:

Testnet block 43561 has 3 Pay-to-script-hash transactions and the /P2SH/ string in it's coinbase:
http://blockexplorer.com/testnet/block/000000000182cc0764d6c6b2b43a009fc9dc0cb03e9002249e7e865b7189acf8

The next block has a transaction that spends one of them:
http://blockexplorer.com/testnet/tx/cff697a07fa21780b2553c6e86bf956cb42838b0e9b226da2c6b3cd7754da736

I'll be producing/testing backports for miners running old software in the next few days.

@gavinandresen
Copy link
Contributor Author

genjix makes a good suggestion: refactor ConnectInputs so fee payment is computed, and transaction possibly rejected, before ECDSA signature checking.

Also sipa's base58 encoding for public keys (compressed or full) aught to be used in the validateaddress/addmultisig RPC calls.

@gavinandresen
Remove not-used-anywhere scriptPrereq from SignSignature()
d11a58a
@gavinandresen
Replace OP_EVAL (BIP 12) with Pay-to-script-hash (BIP 16).
922e8e2
@gavinandresen
Refactored ConnectInputs, so valid-transaction-checks are done before…
8d7849b 
… ECDSA-verifying signatures.
@gavinandresen
Refactor: needing to call ConnectInputs to process coinbase transacti…
4063460 
…ons was counter-intuitive
@gavinandresen
Remove base58 encoding from validateaddress/addmultisigaddress
9e8818e 
base58-encoding of full/compressed public keys needs more thought; it probably makes sense to define a base58 encoding that includes a version byte and a checksum. So just support hex and bitcoin-address encodings for now.
@gavinandresen gavinandresen merged commit 9e8818e into bitcoin:master Jan 13, 2012
@fanquake fanquake mentioned this pull request Feb 2, 2015
Merged
Losangelosgenetics pushed a commit to Losangelosgenetics/bitcoin that referenced this pull request Mar 12, 2020
Alternative at sign @ (ss06) (closes bitcoin#617 closes bitcoin#748 c…
85f4fec 
…loses bitcoin#817)
This was referenced May 3, 2020
Closed
Merged
maflcko pushed a commit that referenced this pull request May 4, 2020
Merge #18859: Remove CCoinsViewCache::GetValueIn(...)
74a1152 
b56607a 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 b56607a
  promag:
    Code review ACK b56607a.
  jb55:
    ACK b56607a
  hebasto:
    ACK b56607a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6
ComputerCraftr pushed a commit to ComputerCraftr/bitcoin that referenced this pull request Jun 10, 2020
@ComputerCraftr
Merge bitcoin#18859: Remove CCoinsViewCache::GetValueIn(...)
04dcc6e 
b56607a Remove CCoinsViewCache::GetValueIn(...) (practicalswift)

Pull request description:

  Remove `CCoinsViewCache::GetValueIn(...)`.

  Fixes bitcoin#18858.

  It seems like `GetValueIn` was added in bitcoin#748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in bitcoin#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 b56607a
  promag:
    Code review ACK b56607a.
  jb55:
    ACK b56607a
  hebasto:
    ACK b56607a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6
ComputerCraftr pushed a commit to ComputerCraftr/bitcoin that referenced this pull request Jun 10, 2020
@ComputerCraftr
Merge bitcoin#18859: Remove CCoinsViewCache::GetValueIn(...)
514b330 
b56607a Remove CCoinsViewCache::GetValueIn(...) (practicalswift)

Pull request description:

  Remove `CCoinsViewCache::GetValueIn(...)`.

  Fixes bitcoin#18858.

  It seems like `GetValueIn` was added in bitcoin#748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in bitcoin#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 b56607a
  promag:
    Code review ACK b56607a.
  jb55:
    ACK b56607a
  hebasto:
    ACK b56607a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6
rajarshimaitra pushed a commit to rajarshimaitra/bitcoin that referenced this pull request Aug 5, 2021
@fdegros
Fix a couple of typos (bitcoin#748)
231ae44 
* Add name to contributors in preface

* Add missing word

* Remove extraneous word

"being in used" -> "being used"
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gavinandresen @sipa