search

bitcoin-dev-project / warnet

Monitor and analyze the emergent behaviors of Bitcoin networks
https://warnet.dev
MIT License
63 stars 28 forks source link

enable sync_all via base class #362

Closed mplsgrant closed 2 weeks ago

mplsgrant commented 2 weeks ago

Issue

Calling sync_all() from within a scenario does nothing.

Cause

WarnetTestFramework explicitly passes on sync_all().

Solution

Enable sync_all() by removing it from WarnetTestFramework which exposes it via the base class BitcoinTestFramework.

pinheadmz commented 2 weeks ago

There is a specific reason this was removed: it requires ALL nodes' mempools and blockchains are exactly the same. That might make sense for bitcoin core test networks but it will be vary common in Warnet to have network divisions or inconsistent mempool policies. It is a default callback for generate() so it may cause infinite waits in some scenarios.

Unless I'm missing something @josibake @mplsgrant I think we should consider reverting this. Perhaps we need to add a test with a split network that would fail if this patch were merged.

Long term: I think since our last call its made me think that scenarios may be our primary interface with warnet users. In that case, we may want to just consider the bitcoin test framework a placeholder and actually write our own scenario framework so we dont have to hack the bitcoin core framework to little bits.

pinheadmz commented 2 weeks ago

consider reverting this

Ok second thought, this is too harsh. But we should make sure all the self.generate() calls in scenarios use a dummy to stub out the sync_fn() argument.

And I still think we should plan long term on replacing the bitcoin test framekwork

mplsgrant commented 2 weeks ago

Ok second thought, this is too harsh. But we should make sure all the self.generate() calls in scenarios use a dummy to stub out the sync_fn() argument.

Agreed

And I still think we should plan long term on replacing the bitcoin test framekwork

I am curious to hear more about this idea. Coming from the outside, it seems like lots of people "think" in terms of the bitcoin test framework. So, supporting it gets us lots of tests that people might code up on a whim and want to run.

pinheadmz commented 2 weeks ago

@mplsgrant starting dsicussion here: https://github.com/bitcoin-dev-project/warnet/discussions/364