Page MenuHomePhabricator

Merge #12885: Reduce implementation code inside CScript
ClosedPublic

Authored by nakihito on Aug 28 2019, 23:16.

Details

Summary

54a5a21 [MOVEONLY] Turn CScript::GetOp2 into a function and move to cpp (Pieter Wuille)
6a7456a [MOVEONLY] Move CSCript::FindAndDelete to interpreter (Pieter Wuille)
33a8ecf Delete unused non-const-iterator CSCript::GetOp overloads (Pieter Wuille)
2fb168b Make iterators in CScript::FindAndDelete const (Pieter Wuille)

Pull request description:

This PR moves `FindAndDelete` and `GetOp2` out of CScript (the first is only used inside the interpreter and moved there, the second does not actually depend on any script specifics and works on any vector). Furthermore, all non-const-iterator versions of GetOp are replaced by const ones, removing a number of methods in the process.

The longer term goal here is making the script interpreter independent from the CScript representation.

Note for reviewers: both `FindAndDelete` and `GetScriptOp` are consensus critical.

Tree-SHA512: c4ccf91c0b33c37cff0d474aa8dd2dab25b5b7655e2ed69a9b15e29daf0a67b21d51c23e1defb3a72ec762bd6138de96f69c6db1fb9c1fe1e976e421261aedb7

Backport of Core PR12885
https://github.com/bitcoin/bitcoin/pull/12885/

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR12885
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 7274
Build 12593: Bitcoin ABC Buildbot (legacy)
Build 12592: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Aug 28 2019, 23:16
src/script/interpreter.cpp
53

This change isn't in the original PR, but required because of here: https://reviews.bitcoinabc.org/D341#change-9hDdQ9uC4uT6

deadalnix requested changes to this revision.Aug 29 2019, 16:58

There are numerous regression in the implementations of the code that this PR is moving.

src/script/interpreter.cpp
77

Braces

This revision now requires changes to proceed.Aug 29 2019, 16:58

Fixed variable typing and added braces.

deadalnix requested changes to this revision.Aug 30 2019, 13:54

You got that wrong. The compiler is warning about it. You need to fix this.

This revision now requires changes to proceed.Aug 30 2019, 13:54

Fixed variable type comparison warning.

deadalnix requested changes to this revision.Sep 12 2019, 07:25
deadalnix added inline comments.
src/script/interpreter.cpp
73 ↗(On Diff #11050)

Putting this in the middle of the two function that cleanup script code is an odd choice. Please reorder so that strongly connected component are adjacent.

This revision now requires changes to proceed.Sep 12 2019, 07:25
src/script/script.cpp
527 ↗(On Diff #11050)

I don't understand why you are adding random changes to code that simply should have moved.

Changed location of FindandDelete() and restored newline.

This revision is now accepted and ready to land.Sep 17 2019, 22:30