WIP: Make CScript addition concatenative #18613

This has come up in some testing related stuff I've been working on... Opening as a WIP because a lot of stuff fails as a result and trying to decide if it's worth fixing or if I should ignore it...

Currently:

CScript(["hello", "world"]) != CScript(["hello"]) + CScript(["world"])

CScript(["hello"]) + CScript(["world"]) == CScript(["hello", "\x06\x05world") == CScript(["hello", CScript(["world"]))

It doesn't seem like there's a way to reach CScript(["hello", "world"]) other than by chaining the iterators together, which is clunky.

This patch redefines addition of scripts such that:

CScript(["hello", "world"]) == CScript(["hello"]) + CScript(["world"])

CScript(["hello"]) + CScript(["world"]) != (CScript(["hello", "\x06\x05world") == CScript(["hello", CScript(["world"])))

Note that adding a raw bytes to a CScript preserves the old behavior, e.g., CScript(["hello"]) + "world" == CScript(["hello", "world"]).

An alternative to this is to get rid of the nested parameter to __coerce_instance and always leave CScript as concatenative -- e.g., CScript([a,CScript([b])]) == CScript([a,b]) == CScript([a]) + CScript([a,b]) and you have to coerce to bytes first to get a push. This breaks fewer tests, but seems a bit more subtle?

Tests should be easy to write, and easy to read. Why can't this nonsensical operator on scripts be marked deleted in favour of self-documenting ones like def push_data(self, vch), def push_opcode(self, op) or so? This way, script operations are easier to follow, but byte operations are not ruled out because (if needed) they can still be done by casting to bytes first.

I'm open to whatever, I just think this is a bit too much non WYSIWYG of an idiom so happy for any replacement.

I've replaced the implementation with raise NotImplementedError and only one test fails. So instead of changing the implementation and having reviewers go through that painful review, with the goal of keeping it around long term, doesn't seem ideal to me. I think it can be replaced by something simpler in the one test that uses it and then ditched once and for all.

For future reference, @MarcoFalke is referring to the line

tx1b.vout = [CTxOut(1 * COIN, DUMMY_P2WPKH_SCRIPT + b'a')]

in test_simple_doublespend of test/functional/feature_rbf.py.

So I guess this can be replaced by a "pushdata" method, e.g. DUMMY_P2WPKH_SCRIPT.push_data(b'a') ?

Or better, since I think it would be natural to assume push_data mutates:

too_long_script = DUMMY_P2WPKH_SCRIPT.copy()
too_long_script.push_data(b'a')

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #18732 (test: Remove unused, undocumented and misleading CScript.add by MarcoFalke)

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.

closing since #18612 and #18732 got merged