Adds a functional test for CPFP with depth 1.
AbandonedPublic
Actions

Authored by ealmansi on Jan 24 2019, 01:54.

Details

Reviewers

deadalnix
jasonbcox

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Summary

Child-Pays-for-Parent: Depth 1.

A child transaction with a sufficiently high fee can lead to
the inclusion of its parent transaction into a block; even if
the block is full and the parent has a fee which is lower than
that of all other transactions in the mempool.

Test Plan

./test/functional/test_runner.py mining_cpfp_depth_1

Diff Detail

Repository

rABC Bitcoin ABC

Branch

add-cpfp-functional-test

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 4678
Build 7419: Bitcoin ABC Buildbot (legacy)
Build 7418: arc lint + arc unit

Event Timeline

ealmansi created this revision.Jan 24 2019, 01:54

Owners added a reviewer: Restricted Owners Package.Jan 24 2019, 01:54

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 24 2019, 01:54

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

deadalnix requested changes to this revision.Jan 24 2019, 02:10

deadalnix added inline comments.

test/functional/mining_cpfp_depth_1.py
77	You need to check that that transaction is rejected. If not then you test nothing.
88	You need to check that the nose will request the parent from you. This means you need to connect to the node via a test node and not via the RPC.

This revision now requires changes to proceed.Jan 24 2019, 02:10

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B4678: Diff 6890.Jan 24 2019, 02:10

@deadalnix could you please clarify on the need of adding an extra test node?

The test as it stands is RPC only (just like 'mining_prioritisetransaction') since my goal was to cover two things:

That adding transactions to the mempool updates the modified fees of its parents. [CTxMemPool::addUnchecked]
That the transaction selection algorithm for mining chooses transactions based on modified fees taking into account the child fees, and not just their own fees. [BlockAssembler::CreateNewBlock]

I don't understand why an additional node would improve coverage of 1) and 2). Or is there a completely different set of behaviours related to CPFP that you think should be tested?

@ealmansi The reason you need the extra test node is because there's a third thing you need to check:

Check that the CPFP transaction and its low-fee parent will propagate correctly on the p2p layer. There's not much point to being able to mine the CPFP transactions in your mempool if they never make it into your mempool. (Also check that the low-fee parent alone does not propagate.)

@deadalnix @jtoomim that contradicts my understanding of CPFP.

I believe the way CPFP works and the way it is used in the wild is by accelerating a low-fee transaction that has already been broadcasted and is in the mempool.

The basic use case (see [1]):

Alice sends a transaction to Bob from her wallet with a fee of L sat / byte, where L > minRelayFee.
There is a large influx of transactions to the network with fees greater than L sat / byte, which would make Alice's transaction unlikely to be included in the next block.
Bob spends Alice's unconfirmed outputs back to himself but with a higher fee, such that the modified fees on Alice's transaction become eligible for inclusion in the next block again.

Note that Alice's original transaction was perfectly valid for being relayed; otherwise, the node at her wallet's backend would have rejected it from the beginning.
The use case described above is what this file is testing.

If my understanding is wrong, please let me know!

[1] https://blog.coinbase.com/improving-bitcoin-reliability-through-child-pays-for-parent-77e771bb04d6

As discussed on Telegram, a different approach is needed.