Details

Reviewers

deadalnix
Fabien
jasonbcox

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Commits

rSTAGINGc023ad5f2890: Merge #12885: Reduce implementation code inside CScript
rABCc023ad5f2890: Merge #12885: Reduce implementation code inside CScript

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 7495
Build 13033: Bitcoin ABC Buildbot (legacy)
Build 13032: arc lint + arc unit

Event Timeline

nakihito created this revision.Aug 28 2019, 23:16

Owners added a reviewer: Restricted Owners Package.Aug 28 2019, 23:16

Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 28 2019, 23:16

Build Bitcoin-ABC / Diffs / Diff Testing started.

Removed typo.

Build Bitcoin-ABC / Diffs / Diff Testing started.

nakihito added inline comments.Aug 28 2019, 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

Harbormaster completed remote builds in B7273: Diff 11013.Aug 28 2019, 23:28

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7274: Diff 11014.Aug 28 2019, 23:33

Build Bitcoin-ABC / Diffs / Diff Testing passed.

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

Fixed variable typing and added braces.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7284: Diff 11035.Aug 29 2019, 20:52

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.

Build Bitcoin-ABC / Diffs / Diff Testing started.

Harbormaster completed remote builds in B7295: Diff 11050.Aug 30 2019, 18:59

Build Bitcoin-ABC / Diffs / Diff Testing passed.

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.