[qa] Multi-chain support in test framework

kallewoof commented at 3:44 am on February 27, 2017: member

This adds multi chain support to BitcoinTestFramework.

(Early PR comments refer to the fact this was originally a new class that would replace the current one eventually.)

fanquake added the label Tests on Feb 27, 2017

ryanofsky commented at 5:22 pm on February 27, 2017: member

I think it would be better to have the new code in MultiChainBitcoinTestFramework replace the existing code in BitcoinTestFramework instead of selectively overriding it, given that it seems to be backwards compatible. All you would have to do is add a new optional named argument to split_network and setup_network and interpret it in the backwards compatible way when it is not specified. I guess I don’t see a reason to add code duplication and complexity with a new MultiChainBitcoinTestFramework class (even temporarily with the “ultimate intention” of merging it later).

Also I think I would prefer a more flexible and predictable mechanism for splitting the network than the chains argument provides. What about a splits argument that takes a list of indices into self.nodes where splits should be inserted into the network? E.g. if you specify num_nodes=6 and splits=[1,3] then node 0 would be standalone, nodes 1-2 would be connected, and nodes 3-5 would be connected. Other advantages of this scheme beyond more flexibility is that I think it would be more explicit and simpler to understand and implement since it wouldn’t involve floating point calculations to figure out what the splits look like.

sdaftuar commented at 7:35 pm on February 27, 2017: member

I tend to think that this is unnecessary (and is just adding complication to the qa codebase). I find the existing is_chain_split stuff already confusing and relatively unused, basically because having to stop/restart nodes in order to change network topology is pretty clunky.

Moreover I think your listsinceblock.py test in this PR is not actually testing what you want; after you reconnect the network there’s no guarantee that node0 will actually download all 3 chains – it might only download the longest chain.

I think there are much easier and more robust ways to write the kind of listsinceblock test that you’re going for; I’ve coded one up here using invalidateblock: https://github.com/sdaftuar/bitcoin/commit/0a10605b782c1b7997833e1a3644cb02a6c437da

My suggestion would be to just PR something like my proposed commit, and drop this MultiChain-test framework stuff.

kallewoof commented at 8:56 pm on February 27, 2017: member

@ryanofsky It’s not backwards compatible with everything, which is why I wanted to temporarily put it aside, instead of fixing everything. I may be overlooking something but if you do the change you suggest, you’ll see that fundraw does some funky stuff which I’m not sure how to interpret using the new method. I’ll dig though. I like your suggestions about where to split the chain though. Will change to do that for sure. @sdaftuar The reason this PR exists is because I was asked to test 3+ chains in #9622 but realized I can’t do this easily. The old set up is clunky, as you say, and even without using the 3+ chain functionality, this change would remove the dependency on 4 nodes.

kallewoof force-pushed on Feb 28, 2017

kallewoof commented at 1:52 am on February 28, 2017: member

@ryanofsky I ended up finding the cause for why fundrawtransaction.py failed, and removed the old framework, leaving only the new one in place. I still intend to change out the splitting part to use your suggested method.

Unsquashed branch (pardon sloppy commit messages): https://github.com/kallewoof/bitcoin/commits/qa-multi-chain-support-unsquashed

kallewoof force-pushed on Feb 28, 2017

kallewoof commented at 6:24 am on February 28, 2017: member

@ryanofsky Switched to using splits array.

kallewoof force-pushed on Feb 28, 2017

sdaftuar commented at 2:14 pm on February 28, 2017: member

@sdaftuar The reason this PR exists is because I was asked to test 3+ chains in #9622 but realized I can’t do this easily. The old set up is clunky, as you say, and even without using the 3+ chain functionality, this change would remove the dependency on 4 nodes.

Did you look at the patch I proposed for implementing that 3-chain test? I believe it is correct (unlike the test provided in this PR) and easier to understand. As I mentioned before, there is no guarantee in your test that node0 will download all 3 chains, which defeats the point of the test!

In general I think using network topology to try to simulate the effect of a node receiving multiple chain forks is at best confusing to reason about, as it requires understanding nuances of block download logic in order to determine what blocks each node would actually download.

Absent any compelling use cases for this test framework change, I tend to NACK.

ryanofsky commented at 2:54 pm on February 28, 2017: member

Yes, you guys should look at each others patches if you have not already. @sdaftuar, not sure if you have seen the newest changes to test_framework.py from @kallewoof:

https://github.com/kallewoof/bitcoin/compare/6206252e5073c1cde2e313f2e5a3ca17582c5823...02647ff75b8ac1d46a9c9048176bccaebc90e9bc#diff-6a670fff9d11003cce8baa0518efd7a9

I think this could be tidied up a little more, but that it makes nice and backwards compatible cleanup to the setup_network / split_network / sync_all code. I think it is good to move in the direction of making these methods actually useful, so more tests can call them instead of overriding and reimplementing them.

kallewoof commented at 4:31 pm on February 28, 2017: member

@sdaftuar I did look at it and intended to respond but was caught up doing other things. The test does not test whether all 3 chains are downloaded at all. It tests whether the transaction, given a reorg, is in its expected place. Whether the client downloads chains or not is completely irrelevant, and in fact is something listsinceblock should not care about – think of it as a reiteration from a given position (block hash) to the current state (chain tip). In this case, it should only care about the chain block hash is in, and the main chain, and no other chains.

In either case, there is no guarantee that invalidating chains and regenerating new ones like you do in your patch will have the same end results as actually physically splitting the network and then merging, is there?

I will look at the patch more in detail to see if I’m missing something.

sdaftuar commented at 4:47 pm on February 28, 2017: member

@kallewoof I’m sorry if I’m misunderstanding the intent here – but if you don’t care whether node0 downloads all 3 chains, then what is the point of test_reorg3 (ie how is it different from test_reorg)? Aren’t you then just testing that bitcoind’s listsinceblock behavior is unaffected by information that is unknown to it? That seems tautological…

In either case, there is no guarantee that invalidating chains and regenerating new ones like you do in your patch will have the same end results as actually physically splitting the network and then merging, is there?

I don’t understand the question – the test I wrote (calling invalidateblock on node1 only, so node0 was not affected) results in 3 chain forks of varying lengths, which I ensure are all downloaded by all nodes. I don’t know what “physically splitting the network” has to do with the listsinceblock test; it seemed to me that you were doing that as a workaround for generating competing chain forks, rather than testing anything directly related to network topologies.

kallewoof commented at 5:01 pm on February 28, 2017: member

To be honest, I was kind of making that test as a POC that the multi-chain code works as intended, aside from the actual multi-chain test script itself. I should have obviously given more thought to it..

A more obvious use case (but still convoluted) is one of the tests I intend to write for #9622 once merged where

0           aaa0
1      /     |     \
2a1         b1       c1-tx2'
3a2-tx1'    b2-tx2   c2
4a3         b3-tx1   c3

There are two double spends in two separate chains. tx1' in a2 is double spent as tx1 in b3, and c1-tx2' in b2. A user does listsinceblock a3 and should not get information about c1-tx2' in the results, but it should get information about a2-tx1'. A lot of discussion about this in theory went into that thread, and someone eventually suggested we make actual tests for this to ensure it works as intended.

I agree to a certain extent that network stuff shouldn’t affect things, but in reality, there is no strong guarantee that using the invalidate chain functionality will have the same effect as actually splitting the network (at least that’s what it looks like, but I’m not entirely familiar with it (invalidateblock) so I need to look into it more closely). There could be bugs or subtle differences that make all the difference in how the nodes communicate and update their wallets or chain states.

jnewbery commented at 5:32 pm on February 28, 2017: member

I’m sorry, but I’m also tending towards a NACK.

I think the split_network() method is too specific and is a trap for test writers that don’t know that it stop-starts all the nodes and may have unexpected side-effects. I’d much rather replace it with a more powerful interface which allows test-writers to control network topology when it’s required. Adding to split_network() is a step in the wrong direction.

For now, I think invalidateblock is probably a better method for creating competing chains.

kallewoof commented at 5:55 pm on February 28, 2017: member

What if it didn’t stop/start the nodes? What other unexpected side-effects does it have?

I’m all for switching, assuming you get the exact same results as if you were splitting for real. What about things like mempool and such? Are these the same with both styles?

jnewbery commented at 6:36 pm on February 28, 2017: member

I’d be a lot happier if it didn’t stop-start the nodes and instead called into the disconnectnode() RPC on the nodes you want to disconnect from each other.

My other concern is that splitnodes() is too specific, and is unlikely to get used in many other tests. I think a better interface would be for the BitcoinTestFramework class to expose a disconnectnodes() method in the BitcoinTestFramework class, so the test writer can create exactly the topology they need.

kallewoof commented at 7:39 pm on February 28, 2017: member

@sdaftuar To better explain the purpose of this feature, I have made a branch which incorporates this and #9622 and adds a test for 3 chains (test_doublespend3) here: https://github.com/kallewoof/bitcoin/tree/nomerge-multichains (test is in https://github.com/kallewoof/bitcoin/commit/22a5509e5c84219074c3cd10e0c1b0916b97daa9)

Am I right in assuming that this can be replicated using invalidatechain and give the same results?
What about mempool trickery? There is a clear difference between fully connected nodes and partitioned nodes in terms of mempool that invalidatechain does not handle, right? Won’t this affect the results?
What other effects would switching to invalidatechain have that could be detrimental to the reproducibility of real life scenarios with partitions/reorgs/sybil attack scenarios/etc?

ryanofsky commented at 8:12 pm on February 28, 2017: member

I’d be a lot happier if it didn’t stop-start the nodes

I don’t object to your overall point, but just would note that we already have split_network function right now that is used in two tests and stop-starts the nodes. Maybe it should be renamed. Maybe it should be made super smart and efficient. But the only thing kallewoof’s branch does to split_network is give it a new optional argument that it passes along setup_network, which doesn’t seem like a bad thing.

[qa] Multi chain support in new MultiChainBitcoinTestFramework class. c0cbb3153e

kallewoof force-pushed on Mar 22, 2017

[qa] listsinceblock.py updated to include a 3-chain version of testreorg. ee97293c68

[qa] Add multichaintest.py for testing the multi chain test framework. 0975369885

kallewoof force-pushed on Mar 22, 2017

jnewbery commented at 4:11 pm on April 12, 2017: member

I’ve just opened #10198 that rewrites split_network() and join_network(). I think it’s a better approach and makes the test framework more generic by not making assumptions about test network topology.

kallewoof commented at 2:05 am on April 13, 2017: member

@jnewbery I’m confused. #10198 doesn’t look like it allows splitting the network into 3+ partitions, which is what this PR is about. Otherwise, it looks like a great improvement!

jnewbery commented at 12:54 pm on April 13, 2017: member

Right. My main point in #10198 is that the test framework shouldn’t have such specific assumptions about the test network topology. If #10198 is merged, then your testcase should just call disconnect_nodes directly, or define helper functions locally.

kallewoof commented at 5:33 am on April 14, 2017: member

@jnewbery I guess you’re right that there is not a lot of reason for testing 3+ chains in other tests. Closing.

kallewoof closed this on Apr 14, 2017

kallewoof deleted the branch on Mar 13, 2018

MarcoFalke locked this on Sep 8, 2021

[qa] Multi-chain support in test framework #9872