Page MenuHomePhabricator

Move OP_CSV from scriptSig to scriptPubKey in bip68-112-113-p2p
ClosedPublic

Authored by Fabien on Tue, Dec 4, 15:30.

Details

Summary

Prepare for magnetic anomaly with push-only rule

Depends on D2146

Test Plan
./test/functional/bip68-112-113-p2p.py

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

Fabien created this revision.Tue, Dec 4, 15:30
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Dec 4, 15:30
Herald added a subscriber: schancel. · View Herald Transcript
Fabien updated this revision to Diff 6274.Tue, Dec 4, 15:32

Remove debug mods in interpreter.cpp

jasonbcox requested changes to this revision.Tue, Dec 4, 17:53
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/functional/bip68-112-113-p2p.py
170 ↗(On Diff #6274)

Two things:

  1. This can be folded into create_test_block with an optional param to spend txs.
  2. Does this function need to increment self.tipheight and self.last_block_time? generate_blocks currently does it, but that seems misplaced now that create_test_block_spend_utxos is being called directly.
This revision now requires changes to proceed.Tue, Dec 4, 17:53
Fabien marked an inline comment as done.Tue, Dec 4, 20:43
Fabien added inline comments.
test/functional/bip68-112-113-p2p.py
170 ↗(On Diff #6274)
  1. It reminds me the discussion from D2131, where @deadalnix asked for removing the bool parameter because bool parameter is an anti-pattern. I could have factorized things slightly more, but the code being added in the middle of the function it won't gain much.
  2. The whole test is assuming that create_test_block won't increment tip height ; as soon as a block is accepted, it is invalidated. Furthermore, the tip height and block time are explicitly set at some points in the test. I think this makes the test flow difficult to figure out, but changing it will require much more changes in the code and is beyond the scope of this diff.
jasonbcox added inline comments.Wed, Dec 5, 19:34
test/functional/bip68-112-113-p2p.py
170 ↗(On Diff #6274)

I didn't catch #2 since I haven't finished reviewing this diff. Thanks for explaining.

Regarding #1, that's a good point. In that case, we should be using composition here. Something like to:

def create_test_block_spend_utxos(self, node, txs, version=536870912):
    block = create_test_block(node, txs, version)
    block.vtx.extend([self.spend_tx(node, tx) for tx in txs])
    block.hashMerkleRoot = block.calc_merkle_root()
    block.rehash()
    block.solve()

It's not a massive improvement, but this way, the tipheight and last_block_time strangeness is contained to just create_test_block rather than duplicated in two places.

Fabien updated this revision to Diff 6285.Wed, Dec 5, 20:10

Update create_test_block_spend_utxos to make it less confusing regarding tip height

jasonbcox accepted this revision.Thu, Dec 6, 02:40
This revision is now accepted and ready to land.Thu, Dec 6, 02:40
This revision was automatically updated to reflect the committed changes.