Page MenuHomePhabricator

Merge #12885: Reduce implementation code inside CScript
ClosedPublic

Authored by nakihito on Wed, Aug 28, 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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nakihito created this revision.Wed, Aug 28, 23:16
Owners added a reviewer: Restricted Owners Package.Wed, Aug 28, 23:16
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Aug 28, 23:16
nakihito updated this revision to Diff 11014.Wed, Aug 28, 23:19

Removed typo.

nakihito added inline comments.Wed, Aug 28, 23:19
src/script/interpreter.cpp
53 ↗(On Diff #11014)

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.Thu, Aug 29, 16:58

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

src/script/interpreter.cpp
77 ↗(On Diff #11014)

Braces

This revision now requires changes to proceed.Thu, Aug 29, 16:58
nakihito updated this revision to Diff 11035.Thu, Aug 29, 20:40

Fixed variable typing and added braces.

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

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

This revision now requires changes to proceed.Fri, Aug 30, 13:54
nakihito updated this revision to Diff 11050.Fri, Aug 30, 18:47

Fixed variable type comparison warning.

deadalnix requested changes to this revision.Thu, Sep 12, 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.Thu, Sep 12, 07:25
deadalnix added inline comments.Thu, Sep 12, 07:26
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.

nakihito updated this revision to Diff 11369.Tue, Sep 17, 18:40

Changed location of FindandDelete() and restored newline.

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