search

mastercoin-MSC / mastercore

mastercore info
mastercoin.org
MIT License
24 stars 11 forks source link

Bitcoin 0.10 has been released - what should we do? #286

Open zathras-crypto opened 9 years ago

zathras-crypto commented 9 years ago

Hi all,

I'd like to get a discussion going with as much participation as possible. The issue is simply that Bitcoin 0.10 has been released, and this is not compatible with OmniCore 0.0.9.1 (due to headers first).

We're gearing up to release the UI, however we need a strategy to handle the issue. We can't have our first major end user release being a failure because we haven't planned for this appropriately.

Thoughts please?

Thanks Z

achamely commented 9 years ago

How does the Omnicore process the omni transactions now. Are they triggered during the normal bitcoin tx processing? If so how about adding a middle man step.

High level thought process: The new headers first method seems to grab multiple chains and determines which one has the most proof-of-work before downloading its actual blocks. Could we, Once the download blocks is triggered, side process and create a standalone list of Omni tx's with corresponding tx/block times. Then once the blockchain data is complete we trigger the full omni parsing of our new ordered list.

dexX7 commented 9 years ago

Every time a new block is connected, it is forwarded to the meta layer at the moment.

For 0.10 instead of forwarding every new block, a block should be forwarded, when it's the very next to the last processed one, and the forwarding should continue, until no next block is available. But there are also "higher level" signals to subscribe to and it's not necessarily required to hook add handlers into ConnectTip (IIRC).

https://github.com/mastercoin-MSC/mastercore/pull/263 worked, back in December, although I tested it basically only fully synchronized with new blocks added to the top.

The biggest hurdle is probably UI related, because it has changed significantly:

0 10

msgilligan commented 9 years ago

From my perspective (someone developing software to the RPC API) the 0.10.0 release is very desirable. Several of the improvements (faster blockchain downloading, RPC "warm-up" mode) address pain points that come up in testing/integrating.

There is also a fix for building on OS X 10.9.

And I almost certainly will update Bitcoin-Qt on my Mac.

zathras-crypto commented 9 years ago

Sorry, for clarification I mean strategically.

So yes absolutely we want to move to 0.10, but that will take time (an unknown quantity at this point).

Moreso things like:

The technical stuff is still important, but moreso end user facing issues such as:

The actual questions surrounding a technical migration would be addressed as we actually worked towards OmniCore 0.0.10. I'm moreso concerned with the here and now - our ability to a) deliver this platform in a working fashion if we release now and b) our ability to support the platform in the context of the incompatibilities between the 9 and 10 branches of Bitcoin.

Hope that makes more sense :)

msgilligan commented 9 years ago

I guess I'm saying I support switching to 0.10.0 before making any new releases -- including the UI. At the very least we should take the time to quantify the effort before deciding.

zathras-crypto commented 9 years ago

So the decision was made to go ahead with a 0.0.9.1 based release, along with some code to force a shutdown during startup if we detect a blockchain from 0.10.

First question now to all - before I spin up the VM's to test, has anyone actually tried to run OmniCore 0.0.9 against a 0.10 blockchain? @msgilligan you mentioned you thought code was already there to prevent 0.9 bitcoin clients running when a 0.10 blockchain is present - have you tested this by any chance?

Thanks all Z

msgilligan commented 9 years ago

I haven't tried it, no. I don't have any specific knowledge, just a hunch given what I know of the Bitcoin core team and the fact that this feature was on their roadmap for a long time.

The Downgrading section of the 0.10.0 release notes says "It is possible that the data from a completely synchronised 0.10 node may be usable in older versions as-is, but this is not supported and may break as soon as the older version attempts to reindex."

zathras-crypto commented 9 years ago

OK great, thanks for the info - that's kind of what I was concerned about - have a bitcoin 0.10 install and run Omnicore which may "break as soon as the older version attempts to reindex" - that kind of indicates to me that an older version will attempt to reindex, rather than cleanly shutting down.

I'll have a poke around and see what I can come up with :)

Thanks Z

dexX7 commented 9 years ago

Just playing devil's advocat here for the sake of being overly protective: the risk is that blocks are stored out of order, because they arrive out of order, and an old client may not make sense of it. I assume that this risk is also given, if the user is just a few blocks behind and catches up.

Blocks will be stored on disk out of order (in the order they are received, really), which makes it incompatible with some tools or other programs. Reindexing using earlier versions will also not work anymore as a result of this.

Reindexing and fetching the last blocks doesn't seem much different.

Edit: this was only a guess, but after thinking more about it: blocks are indexed, and a 0.9 client wouldn't necessarily locate blocks by scanning files, but rather looking into the blockdb, so the statement regarding reindexing gains more significance for me now and it is also a bit different than catching up.

@zathras-crypto: do you have an idea how to identify it?

zathras-crypto commented 9 years ago

Alrighty, testing this out myself ATM. Have taken a snapshot, deployed 0.10 and spun it up. Firstly no idea why it needed to write over a GB of delta when 'verifying blocks' - assuming it's doing more than just verification there.

It still appears to be syncing in sequence (days going down one by one, blocks processed going up sequentially about 50 blocks at per update).

zathras-crypto commented 9 years ago

OK. I performed the following test:

First thing to confirm, there is absolutely no detection or protection against using the bitcoin 0.10 blockchain with an earlier client (at least bitcoin 0.9.3 (our base) anyway). OmniCore fires up no errors or warnings.

zathras-crypto commented 9 years ago

Still nosing around looking for a way to detect if 0.10 has been used on the blockchain, but I did notice that even on 0.10 the blocks seemed to be processed in sequential order. Perhaps this is because peers aren't running 0.10?

http://pastebin.com/raw.php?i=2hRmGrp5

dexX7 commented 9 years ago

You should try getblockchaininfo, but it looks like you're already fully synced now. It shows the number of headers as well blocks.

dexX7 commented 9 years ago

So the decision was made to go ahead with a 0.0.9.1 based release, ...

Not that I want to hijack this decision, but I was curious about the block connection behavior and ended up porting Omni Core without UI to Bitcoin Core 0.10 in the last hours.

All regtests passed and at the moment I'm synchronizing on mainnet and dump every handler call. Looks good so far! And it appears that ConnectTip is triggered, once there is really a new, next, block:

omni_tips

I'm away for a few hours now, but I'll try to push everything wrapped into expressive commits later.

That being said, porting was so much easier than assumed, but only because I prepared most of it earlier, e.g. cleaning up the includes etc., ... :)

But key was also to start with 0.10 and add the components one by one, instead the other way around as last time, which came with like 100 merge conflicts. >_<

As heads up, those are requirements:

... and my current build also includes some of the other PRs, right now to ensure it also passes the other tests I have.

The incompatibility issues can be resolved in ~15-30 lines or so in total, so reviewing the PRs is probably most of the work for Omni Core based on Bitcoin Core 0.10 without UI modifications, but plain Bitcoin Core interface. Edit: to clarfiy: graphical interface. All the RPC interaction work fine.

@zathras-crypto: what do you think about porting the QT code? As very rough guess I'd assume the tricky part is to deal with 0.10's new interface, while the code blocks could probably be copied one to one. Will take a look at this later as well, if I find some time..

zathras-crypto commented 9 years ago

But key was also to start with 0.10 and add the components one by one, instead the other way around as last time, which came with like 100 merge conflicts. >_<

Hehe, that's also Michael's preferred methodology :)

All regtests passed

I think the primary issue is that I don't think anyone is comfortable changing our underlying bitcoin version in a minor release. It's GREAT to know this can be progressed in such a straight forward manner, and well done my friend :) but when looking at a risk angle - to play the safe card we need to change our base Bitcoin code in a major release not minor.

To be frank, the issue is driven by timing primarily. The powers that be want the UI released yesterday and putting whatever QT work is required aside for a moment, I would still insist on a whole bunch of testing for a new base bitcoin version. I really do appreciate the testing you've done so far, but for me I just don't feel comfortable enough yet to say "in 100% of potential circumstances there is absolutely no possible change to Omni data in bitcoin 0.10" which is needed to do a non-major release.

Sorry, not wanting to ramble on - long story short the main driver is to do 3 things: 1) Clean up the last criticals - signalling - potentially via your PR. Apologies for the delay there (sadly I'm in and out of hospital for various tests at the mo which is quite a time drain, but health has to come first) 2) Put in detection for a 0.10 blockchain and refuse to start (safety net) 3) Release the UI as 0.0.9.1

Once that's done, we could even skip 0.0.9.2 and move straight to a 0.0.10 with bitcoin 0.10.

what do you think about porting the QT code?

To be honest I don't think it'll be a major drama, I'll be applying similar logic to you & Michael with the copying files over - though there may need to be some time spent in QT designer adjusting some of the forms (that's always fun! /sarcasm)

zathras-crypto commented 9 years ago

I'm honestly having quite a hard time trying to find a way to detect a 0.10 from the blocks or chainstate programatically.

So far I can quite easily identify it by checking if the debug.log has certain elements that were introduced with 0.10, but that does not feel like a safe or solid way to do it.

Any suggestions welcome :)

Thanks Z

dexX7 commented 9 years ago

I'm in and out of hospital for various tests at the mo which is quite a time drain ...

Oh damn, I hope you're well!

I think the primary issue is that I don't think anyone is comfortable changing our underlying bitcoin version in a minor release.

Please don't get this wrong, but given that there was not a single announcement on either one of the mailing lists, the forums or the blog about the last consensus critical release, I'm wondering: which release process?

The regtests are by far not complete and relying on them only would be crazy at this point, though it's unfortunally the only route of testing, besides consensus checks, that is currently available. One reason I'd value increasted testing is to replace "feeling it may or may not work" by "knowing this or that definitely works, confirmed by tests". A sort of long way still.

As mentioned earlier, I fully support the conservative approach, but I'm happy about the realization it worked out better than expected. :) But I also agree that pushing the UI release should be highest priority!

Hehe, that's also Michael's preferred methodology :)

Actually this is should only be a workaround and not providing the full commit history is a no-go in my opinion, but it should be possible to cheat here: 1) get the desired result, 2) merge from scratch to preserve history, 3) see tons of conflicts, 4) replace files with desired result from earlier, 5) commit.

Back to the actual issue:

It would certainly help to know what risk there might be, to shutdown as consequence, if things go south. I assume your 0.10 datadir is synced now? If so, and if you don't mind, I'd be very curious about what would happen, if it's used in combination of reindexing and Omni Core 0.0.9.1.

The two relevant questions in this context:

I'm not really sure how the reorg handling and persistence layer of Omni Core comes into play here, but it's guaranteed that no new transaction reaches the meta transaction processing, if the block it belongs to isn't the a very next to the earlier one.

See the assertion at the beginning: mscore-0.0.9/src/main.cpp#L2022-L2024:

// Connect a new block to chainActive.
bool static ConnectTip(CValidationState &state, CBlockIndex *pindexNew) {
    assert(pindexNew->pprev == chainActive.Tip());  //<! THIS ONE HERE

    (void) mastercore_handler_block_begin(GetHeight(), pindexNew);

A few lines later is the actual transaction handler call, which won't be reached by out of order blocks: mscore-0.0.9/src/main.cpp#L2068-L2073:

    BOOST_FOREACH(const CTransaction &tx, block.vtx) {
        SyncWithWallets(tx.GetHash(), tx, &block);
        if (0 == mastercore_handler_tx(tx, GetHeight(), tx_idx++, pindexNew )) ++countMP;
    }

    (void) mastercore_handler_block_end(GetHeight(), pindexNew, countMP);
zathras-crypto commented 9 years ago

Oh damn, I hope you're well!

Thanks dude :) Just got results for CT - taking a glass half full attitude - still don't know what's wrong but given what the Doc suspected I actually shed a tear (call me a p*ssy if you will hehe) when I rec'd the all-clear for that particular diagnosis this morning - sadly more tests now to figure it out but it's pretty darn relieving when the Doc suspects bad things and the tests come back clear :) image

Please don't get this wrong, but given that there was not a single announcement on either one of the mailing lists, the forums or the blog about the last consensus critical release, I'm wondering: which release process?

If there was a way to +1 that comment enough that people would take notice, I would! Honestly mate, I've been raising the issue of a lack of process for release for some time now it just doesn't seem to get much traction :( though I honestly consider it one of the most important pieces to a successful progression on this project.

Back to the actual issue:

Yeah agreed - trying to play the conservative approach but it's really not that easy to identify whether blocks have been written out of order. I do have a methodology but it's loooooooooong-winded and I don't want to use it (it involves looping CBlockIndex for each block and looking up the blk???.dat file it's stored in along with it's byte position and undo locations in the rev???.dat files and then ensuring they're sequential from the previous block). Also scraping debug.log works but isn't what I'd call solid (eg what happens if user deletes that file). I was actually surprised as I thought there would at least be some change to CBlockFileInfo regarding the new way of doing things but it seems not sadly.

Could running Omni Core 0.0.9.1 do any harm to existing 0.10 data?

I don't believe it would, but then that's not backed up by anything other than assumption on how certain things work.

If something goes wrong, e.g. the next block can't be found due to the out of order storage, could it affect the meta layer or would it crash/fail earlier?

Well I was actually chatting with @m21 about that this morning, he made a suggestion of keeping our own eye on things in our own handlers so we could thus make sure to abort the node if for some reason the block handler was called on an out of order block. I don't think it solves the issue because the issue is more one of older clients being able to get at out of order blocks rather than processing out of order (as you note, connecttip should always be called sequentially otherwise core would be processing bitcoin transactions for which the inputs didn't exist yet) but I still think it would be a nice sanity check to drop in given the ease of doing so.

I continue to believe the path of least resistance is to block 0.10 users from using OmniCore 0.0.9.1 and get it released, and then just put all our effort into a OmniCore 0.0.10 branch that uses the new bitcoin 0.10 base and do the requisite analysis and testing while we're not being pressured to release. Just getting there seems to be more of a PITA than I first thought hehe.

Thanks Z

zathras-crypto commented 9 years ago

Figured I'd commit what I'm working on at the mo so you can take a look mate - basically now I'm avoiding loading the blocks from disk and purely using the index it's a lot faster - would still like a better way to do it but this is what I'm poking around with and testing out atm https://github.com/zathras-crypto/mastercore/commit/325a3b959e672991ca8f6c242172c2626b0060a5

Thanks Z

zathras-crypto commented 9 years ago

FYI: image

msgilligan commented 9 years ago

A few 0.10.0 goodies related to RegTest mode that I just discovered:

https://github.com/bitcoin/bitcoin/pull/5275 (RegTest performance improvements) https://github.com/bitcoin/bitcoin/pull/5280 (setmocktime timestamp RPC, could be useful for crowdsale testing, etc.)

dexX7 commented 9 years ago

Ah, great progress!

First off, again, @zathras-crypto: hope you're well!

I tested this approach and it works! Neat idea of checking it this way and I can confirm out of order storage of my 0.10 files. I'm very curious what happens, if I resync 0.9 now .... hehe.

Just wondering about all the vectors and maps? Check this out:

https://gist.github.com/dexX7/807cb7a0e26383d616d0 https://gist.github.com/dexX7/b8b47abaddf3619c3432

While thinking about this, and I really have no idea if it's feasible: would it be possible to copy the reindex logic of 0.10 to support out of order storage, if it turns out this is the primary issue?

@msgilligan: Yeah, the block generation is notably faster. :) Ah and as a side note: the local consensus checks worked fine for me.

dexX7 commented 9 years ago

Using 0.9 as it is works well, even when reparsing all Omni transactions, but reindexing with 0.9 causes a segfault.

zathras-crypto commented 9 years ago

First off, again, @zathras-crypto: hope you're well!

Thanks mate :)

Just wondering about all the vectors and maps? Check this out:

Both of those gists look like interesting methods to simplify what I have mate, nice - let me take a poke around them :)

FYI that code is a reimplementation of PrintBlockTree which is where the maps/vectors come from - probably not necessary (hadn't optimized at all, only got as far as getting something that 'works'). I had the idea on the 'how', and wrote my own code for it but it was slooooow as anything because I was loading each block from disk to analyze. So I went hunting for a method I could use to just do it via the blockindex. Some grepping later I found that the bitcoin devs had the PrintBlockTree function which looked like it would serve as an excellent starting point for this, so I cloned the function and modified it to do what we wanted.

While thinking about this, and I really have no idea if it's feasible: would it be possible to copy the reindex logic of 0.10 to support out of order storage, if it turns out this is the primary issue?

How would you rate the value of this? In my head I had a kind of "get 0.0.9.1 out and then shift all our resources into a 0.10 based branch", so I hadn't really envisioned putting too many more resources behind a 0.9 based branch after 0.0.9.1.

Using 0.9 as it is works well, even when reparsing all Omni transactions, but reindexing with 0.9 causes a segfault.

That's quite interesting actually. So an end user may get away with it (if we didn't have the abort in place) as long as they already had a txindex, but a user who uses a fresh bitcoin 0.10 and then fires up Omni (which enables txindex and thus needs a reindex) would see a segfault?

dexX7 commented 9 years ago

Impressive. The following commit works out of the box with 0.9 and resolves the reindex issue. Maybe it's too early to say, but I feel very confident that this can be used for 0.0.9.1 to handle out of order blocks and Bitcoin Core 0.10 files: https://github.com/bitcoin/bitcoin/commit/ad96e7ccd94679d9125a436ceb5b476aacdfca82

Edit: and the log entries confirm:

UpdateTip: new best=00000000000133c15396bdc0deada3b92e1853cf4758aeb2164bd43a382c723c  height=103358  log2_work=59.250183  tx=236822  date=2011-01-18 20:39:45 progress=0.001802
ProcessBlock: ACCEPTED
LoadExternalBlockFile: Out of order block 00000000000262da964b17196f4a7408c8a2f012d3ac4f41992cfa97b67b4516, parent 000000000000a93b9824a7c979a5d04a9bb23d320f92999d1fada4758e84f716 not known
LoadExternalBlockFile: Out of order block 0000000000005c338692caf0258e42d6388057422622cb03371d534048db7814, parent 000000000002b73753cffd2684a870427988075d6b23a5c75d7b94d70a0b265c not known
LoadExternalBlockFile: Out of order block 000000000002ec3b6e04134c93c55e0310367a402b9fde62b0fdaa23a2012c04, parent 0000000000018a17b2d6a6874492aaac0363875c3b9f69b95bdc4cf73493b0ce not known
LoadExternalBlockFile: Out of order block 0000000000018fa3de766361a49c5f532ce526a4685ae59ddc4e8ac2f9a49531, parent 0000000000010d98058c41320451bb56d5814e240884fb7f8f5a19155a9b1c0c not known
LoadExternalBlockFile: Out of order block 00000000000219064a40eb11dbf8c26d07525a85cf55eaa7a96d74e496a0983c, parent 0000000000015e968f5536142bf7ab1aca037e4f3f551d173bcd9b65f98ea4fe not known
LoadExternalBlockFile: Out of order block 000000000000706a27683dc58eb601c1fdd75e8ebb87fecd005e96de9cb6d4ca, parent 00000000000055c5040a4ef0e3ae5bf8bcdd2ac4587d7088924a411b4fa1a13b not known
LoadExternalBlockFile: Out of order block 000000000000e8731e88ec9cc75815cc13b3eed6971c14e34cb64b2c4f5aa120, parent 000000000002206de2adbb58eabed28c6e92ea1c2d79fe51cde060564d24cfba not known
UpdateTip: new best=00000000000028bfe108436f40a8ead0482fff32e7b3ac76a0c37c24cf7952cf  height=103359  log2_work=59.25035  tx=236825  date=2011-01-18 20:51:20 progress=0.001802
ProcessBlock: ACCEPTED
LoadExternalBlockFile: Processing out of order child 000000000000f64bcec2e37569f83b920df870fbacad7dbf9bf4a95ed6ad31a1 of 00000000000028bfe108436f40a8ead0482fff32e7b3ac76a0c37c24cf7952cf
UpdateTip: new best=000000000000f64bcec2e37569f83b920df870fbacad7dbf9bf4a95ed6ad31a1  height=103360  log2_work=59.250516  tx=236826  date=2011-01-18 20:55:40 progress=0.001802
ProcessBlock: ACCEPTED
LoadExternalBlockFile: Processing out of order child 00000000000241e2433a0ac6dd3b55cc90937eb65078e090e6bf79f3baed5aaa of 000000000000f64bcec2e37569f83b920df870fbacad7dbf9bf4a95ed6ad31a1
UpdateTip: new best=00000000000241e2433a0ac6dd3b55cc90937eb65078e090e6bf79f3baed5aaa  height=103361  log2_work=59.250683  tx=236846  date=2011-01-18 21:07:16 progress=0.001802
dexX7 commented 9 years ago

I'm going to fully resync reindex now and check the final results. If this is turns out to be successful, I suggest to adopt the reindexing code, but fire a warning nevertheless, if a 0.10 chain is detected during the start.

This is less restrictive than forcing a shutdown, and - based on the assumption that the actual risk is related to reindexing - it should be safe, even if the warning is ignored.

As final test, it would be very good to know, if synchronizing (not reindexing) from scratch works as well with this commit.

There seem to be five user groups who may use Omni Core:

The handlers are furthermore guarded against out of order blocks, so, at least regarding the state of Omni/Master, the risk potential seems low.

But I'm still not entirely sure, if there are other side-effects.

zathras-crypto commented 9 years ago

FYI from a discussion I had with @m21 my view was essentially:

the key takeaway for me is that bitcoin devs say of 0.10 "the block files and databases are not backwards-compatible with older versions of Bitcoin Core" which says to me we do not want to use 0.9.3 omnicore against 0.10 blockchain

That's me being risk-averse (probably my background in infra mgmt :P). However when considered in the context of:

It is possible that the data from a completely synchronised 0.10 node may be usable in older versions as-is, but this is not supported and may break as soon as the older version attempts to reindex.

So I guess it comes down to the question of if the bitcoin devs don't want to support using 0.9.x clients on a 0.10 blockchain, do we? I do concur with your analysis above though and the note specifically relates to reindexing breaking, so if we fix that the risk does appear low.

EDIT "completely synchronised" - what about a node that's not completely synchronised? Ie user starts with a 0.10 node, synchronizes half of it (so has a complete set of headers completed via getheaders but only half the blocks via getdata) and then fires up 0.0.9.1

zathras-crypto commented 9 years ago

The following commit works out of the box with 0.9 and resolves the reindex issue. Maybe it's too early to say, but I feel very confident that this can be used for 0.0.9.1 to handle out of order blocks and Bitcoin Core 0.10 files: bitcoin@ad96e7c

I notice that commit is based on top of the headers first PR - is it safe to cherry pick it without the underlying PR?

dexX7 commented 9 years ago

Yes, cherry-picking should in work, but it created conflicts for me, but only limited to the changes itself. Otherwise copy and replace by hand.

Regarding 0.9 in general: it's notable that rather sooner a softfork related to BIP 66 kicks in, see:

https://github.com/bitcoin/bips/blob/master/bip-0066.mediawiki#deployment

This doesn't make 0.9 unusable, but I tested it earlier on regtest and as result a yellow warning in the UI is shown:

Warning: This version is obsolete, upgrade required!

You may spot entries such as the following in the debug.log:

SetBestChain: 4 of last 100 blocks above version 2
dexX7 commented 9 years ago

Either way, I think pushing the release as soon as possible should be a goal, and as next step the migration to 0.10, which appears to be rock solid (appears.. hehe) as server build, in terms of mainnet consensus and all tests that are available.

dexX7 commented 9 years ago

Once 0.0.9.1 is finalized, I could prepare and push the 0.10 based server build without UI.

@msgilligan: I was actually surpirsed about the actual speedup, which was less than expected: the numbers I came up with are roughly - when running all regtests in my build environment:

*It loos like the client responds the RPC requests, even though the UI is "behind".

dexX7 commented 9 years ago

A general question, assuming Bitcoin Core 0.10 becomes the new code base in the near future: does this equal abandoning 0.9 users? Maybe this is not an issue at all, but given that Bitcoin Core 0.9 and Bitcoin Core 0.10 are not really compatible (out of order block storage!), it's probably worth to discuss.

If I'd need to choose between 0.9 and 0.10, I'd use 0.10 of course, but ideally there would be "long term" support for older code bases. It would probably be a PITA at first, and requires sort-of hack-ish wrappers, but here is an example how it could be done: https://github.com/mastercoin-MSC/mastercore/pull/266#issuecomment-68730932

Working towards making Omni Core to be more like a "plugin" for Bitcoin Core is probably a natural goal and byproduct over time, to minimize the impact of changes to the underlying code base, but the whole topic basically starts right now, when migrating to Bitcoin Core 0.10 becomes more tangible.

zathras-crypto commented 9 years ago

I tend to think of following the path of Bitcoin Core development. Yes it provides for a certain degree of influence from an external party but that decision was made I think when we decided to fork Bitcoin Core. So in essence if Bitcoin Core is not offering backwards compatibility between v0.9 and v0.10 blockchains, we thus inherit that as part of the status quo, rather than attempting to add in our own tricks which increases complexity (and thus things that could go wrong).

I think the concept of "long term" support for older code bases is a dead concept at the moment, as long as we keep adding features that invalidate the data provided by older clients we must therefore inherently discourage rather than support their use. Perhaps when we have figured out a way to make Omni features live without breaking older clients we could put some effort into long term support, but for now I think as long as we intend to keep breaking consensus with feature releases our priority must be getting that new code into the hands of users with as much coverage as possible.

I have a tendency to go on :) tl:dr; I suggest that once we move to a Bitcoin Core 0.10 base we should not attempt to try and support 0.9 based blockchains.

dexX7 commented 9 years ago

So in essence if Bitcoin Core is not offering backwards compatibility ...

The main difference is that very outdated Bitcoin clients are still usable, and no one is forced to upgrade. I'm was sure, if this is a concern at all, thus the question, and if there might be any reasonable reason to not upgrade an outdated (Bitcoin) client.

zathras-crypto commented 9 years ago

Ahh sorry I see what you mean, supporting older Bitcoin Core bases with newer Omni Core code. I guess this could be achieved if the Omni Core code was more atomic and independent but again probably not too many use cases for using newer Omni code with outdated Bitcoin clients that I can see :)

dexX7 commented 9 years ago

Bitcoin Core 0.10 based branch published:

https://github.com/dexX7/bitcoin/tree/omnicore-0.0.9.2-no-history https://github.com/dexX7/bitcoin/commits/omnicore-0.0.9.2-no-history aeb92792281b4cb9958f3defc9e36f63e65b778a...dexX7:omnicore-0.0.9.2-no-history

If those changes are acceptable, then I suggest to create a second branch with full commit history and replace Upgrade: add Omni Core v0.0.9.1-rel files, which could then be used to show zero difference.

Once that's done, the other missing parts could be added, e.g. the docs, QT, ...

zathras-crypto commented 9 years ago

Bitcoin Core 0.10 based branch published: https://github.com/dexX7/bitcoin/tree/omnicore-0.0.9.2-no-history

@dexX7 in this context 0.0.9.2 is based on 0.10? If so I'd like to bump the version to 0.0.10 for clarity

dexX7 commented 9 years ago

Yes, it's Bitcoin Core 0.10, but that position was reserved for consensus affecting changes, I assumed? Still not sure, how all this could fit together, because we basically have the following relevant parts:

File names are not strictly relevant in the context of a version number, but updates of the others can have significant consequences for users, and ideally all this would be reflected by the version number. In this case the code base was updated, which is not backwards compatible, but it's also not a mandatory update.

zathras-crypto commented 9 years ago

I'd just been playing the simplistic game - our blockchain 0.10 is going into our next major release, and 0.0.10 is our next major release :)

Re the branch, I have yet to go through them yet but diffing the mastercore* files against their originals from 0.0.9.1 comes up with a bunch of changes - what source did you use for this?

zathras-crypto commented 9 years ago 
[3:14:12 PM] zathrasc: just starting to look at them now
[3:15:03 PM] zathrasc: so looking at mastercore.cpp
[3:15:11 PM] zathrasc: reordering of includes/namespaces
[3:15:31 PM] zathrasc: change to testnet/regtest isNonMainNet function
[3:16:11 PM] zathrasc: change from mscore_parse to GetScriptPushes
[3:16:52 PM] zathrasc: change from mscore_getHex to HexStr
[3:17:24 PM] zathrasc: change mscore_parse again for multisigs to GetScriptPushes
[3:17:59 PM] zathrasc: change manually defined map to BlockMap type
[3:18:44 PM] zathrasc: change to relay and dust limits calcs
[3:19:08 PM] zathrasc: change to using GetScriptForDestination
[3:19:28 PM] zathrasc: mscore_parse actually removed
[3:19:40 PM] zathrasc: ok fair enough dude definitely see your point
[3:20:32 PM] zathrasc: those are not the vanilla files from 0.0.9.1 merged into 0.10 there are other changes in there that haven't been merged in through the usual places
[3:20:38 PM] zathrasc: they might be necessary for the move to bitcoin 0.10

Chatting with @m21 about this branch @dexX7 it looks like there are extra changes in there - I haven't looked at them in detail just a quick skim - are all these required for the migration to bitcoin 0.10?

dexX7 commented 9 years ago

I'd just been playing the simplistic game

Works for me, but what I was really wondering for quite a while: what is the very first version position going to be used for?

... diffing the mastercore* files against their originals from 0.0.9.1 comes up with a bunch of changes

You have to compare the files of the first commit:

This should match with the files of 0.0.9.1-rel.

From there I added the required changes to work with Bitcoin Core 0.10:

zathras-crypto commented 9 years ago

You have to compare the files of the first commit: From there I added the required changes to work with Bitcoin Core 0.10:

Ahh - perfect thanks - that helps.

dexX7 commented 9 years ago

they might be necessary for the move to bitcoin 0.10

Yup, this is the case - it does not contain other "updates", but only the changes required for compability.

The biggest one is the one related to header includes and directives, which was based on my old PR, and I'd be happy to reduce or improve it, but there is no way around fixing that.

zathras-crypto commented 9 years ago

Yup, this is the case - it does not contain other "updates"

Heh, I'm clearly too trusting an individual as the thought didn't even occur to me to diff for changes - probably good I'm not lead on this thing or we'd be in trouble!!!

The biggest one is the one

Cool cool - I chatted with @m21 about this a fair bit today (he's pretty smashed with Factom, they just announced their first crowdsale so things are a bit manic) - anywho we chatted and he's happy with your methodology to go ahead, I'm just going to do the diff on the original commit and then walk the new commits (already scanned them, all harmless) then we should be good to go :)

dexX7 commented 9 years ago

Awesome. :)

Also relevant:

https://github.com/bitcoin/bitcoin/compare/0.10...dexX7:omnicore-0.0.9.2-no-history

Then click on Showing 30 changed files with 11,853 additions and 0 deletions. to show the file list. From there it's easier to confirm there are no changes made to Bitcoin Core or the changes that were made to the non-mastercore files (the ones without green icon) are legit.

dexX7 commented 9 years ago

I pushed a version with full Bitcoin and Omni Core commit history:

The process:

  1. Clone Bitcoin Core 0.10
  2. Merge Omni Core 0.0.9.1-rel
  3. Observe tons of conflicts
  4. Replace all files with those of the first commit ("add omni core files")
  5. Commit
  6. Cherry-pick and sign the rest of the compability patches

I labeled it as strange, because the code bases basically grow in parallel, but are out of sync until the point where the Omni Core files are added to Bitcoin Core. This is expected, but well, strange, at least for me. ;)

@msgilligan: would you do it differently?

zathras-crypto commented 9 years ago

@dexX7 I'd like to get the bitcoin 0.10 based branch going as a start to a 0.0.10 branch on https://github.com/OmniLayer - that will become our master 0.0.10 branch and we can work forward from there - thoughts?

dexX7 commented 9 years ago

@zathras-crypto: I'm already keeping a synchronized branch for the OP_RETURN work, so no worries about wasted efforts, but I agree, the sooner the better.

Can we first agree upon whether the process mentioned in https://github.com/mastercoin-MSC/mastercore/issues/286#issuecomment-82194991 makes sense?

I'll give it another try in a few minutes.

zathras-crypto commented 9 years ago

Well sadly GitHub process isn't my strongest suit mate, but I had discussed the process with Michael and agreed it essentially as:

TL:DR; I thought you basically had it with

You have to compare the files of the first commit: Upgrade: add Omni Core v0.0.9.1-rel files [aa07e74] This should match with the files of 0.0.9.1-rel. From there I added the required changes to work with Bitcoin Core 0.10: Commit history

Basically as long as we have a way to track the history of what's been changed I'm comfortable :)

dexX7 commented 9 years ago

So just to clarify: the Bitcoin Core 0.10 based branch is basically ready:

That's the one you took a look at and it's last state is also the one I'd build upon.

It contains the full Bitcoin Core commit history and the commits I made, starting with the "copy mastercore*" files commit, followed by the upgrade patches.

What I intended was to restore the whole commit history of the Omni/Master Core related commits as well, which I consider as nice to have. Alternatively, I could edit the commit message of the "copy commit" and add a link to the last commit of which those files were based on. This would be doable within a few minutes, but that comes at the price of no Omni/Master Core related commit history prior this point.

As next step, one of the owners, namely @CraigSellars or @achamely, would have to copy/clone/mirror the current mastercoin-MSC/mastercore to OmniLayer/omnicore, or create the repo by some other means, so the 0.10 based branch can be pushed.