mastercoin-MSC / mastercore

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

Release: binaries and release process #292

Closed dexX7 closed 9 years ago

dexX7 commented 9 years ago

Ideally binaries would be created deterministically via Gitian, which is feasible to some degree.

Due to the rebranding from Bitcoin Core to Master/Omni Core, some adjustments need to be made though.

I basically see three options in general:

Option 1 would be anything but trivial (#289), and I really don't like option 3 for a lot of reasons, so I'm going to focus on 2, and how the release could be handled:

  1. Finalize 0.0.9.1
  2. Merge into upstream (mastercoin-MSC/mastercore or OmniLayer/omnicore?)
  3. Tag the release
  4. Setup build environment in a VM (see: slightly outdated tutorial, which I could update)
  5. Locally revert the file name changes (see: 419045f8a022b9dfd482cd3245331fbe34aa6a24)
  6. Create binaries for Unix, Windows, and ideally Mac
  7. Rename binaries back to mastercored, mastercore-qt, ...
  8. Check that at least two parties come up with identical results
  9. Create file checksums and ideally GPG sign the files
  10. Publish binaries, checksums and signatures
  11. Announce release (blog, forums, mailing lists, ...)

I'd prefer a release based on Bitcoin Core 0.9.4, especially because 0.9.3 misses the BIP 66 switch over logic etc. for the upcoming soft-fork, but this seems to be out of the scope of this issue.

Going this route would provide at least deterministically build binaries for Unix and Windows, and given that the UI release seems to target the end user base instead of integrators, stepping up here seems reasonable and justified to me.


@CraigSellars: I agree that option 2 is the most expedient/reasonable (and let’s use mastercoin-MSC/mastercore for this).


@zathras-crypto: Not sure where my notes ended up here - perhaps wrong thread or a missed click on comment :(

TL:DR; option 2 sounds most viable for me also. Had a few other comments, most not terribly important but one thing I was interested in is trust related to GPG keys - eg I have a GPG key I setup with Faiz a while ago, but how does Joe Q end user know it's my key?

zathras-crypto commented 9 years ago

I could do Linux binaries just via Ubuntu the same way I did for 0.0.9?

dexX7 commented 9 years ago

Yup, sure. :)

zathras-crypto commented 9 years ago

Sorry for delayed response mate, crazy busy weekend - yep cool I'll knock out the Ubuntu binaries today along with a suitable readme for binaries etc.

zathras-crypto commented 9 years ago

OK, think these will go into the 'release' unless any objections - merged some doc into a readme and included a license, everything else as per your notes @dexX7 (thanks):

http://omnichest.info/files/omnicore-v0.0.9.1-rel-linux-x64.zip http://omnichest.info/files/omnicore-v0.0.9.1-rel-win-x64.zip

dexX7 commented 9 years ago

Awesome, looks perfect! File hash sums are fine as well! :)

Good to go imho!

zathras-crypto commented 9 years ago

Alrighty here we go https://github.com/mastercoin-MSC/mastercore/releases

dexX7 commented 9 years ago

zathras-crypto released this 3 days ago

Are you a time traveller? ;)

Some nitpicking, but of course everything in my opinion: I started with a list, but figured it would be easier to just create an example: https://gist.github.com/dexX7/0dec2608c500b52b0d17

zathras-crypto commented 9 years ago

FYI https://docs.google.com/presentation/d/1kieBF4ObunFkyYy4SFMnBCNCZQMNpKbyTtRGyHvTn6c/

Looks like the reordered checks are stopping the UI from reindexing itself.

zathras-crypto commented 9 years ago

I don't get the crash, but I do see this error if txindex=1 is not in the conf on a fresh install

zathras-crypto commented 9 years ago
Fresh install, no txindex=1:     Offer to rebuild transaction index, clicking OK shuts down client
Fresh install, txindex=1:        Startup OK
Existing install, no txindex=1:  Warning about disabled txindex detected, clean shutdown
Existing install, txindex=1:     Offer to rebuild transaction index, clicking OK starts rebuild

So looks like the first scenario has a bad flow here the rest seem OK.

zathras-crypto commented 9 years ago

Looks like if we move this check


    if (!fTxIndex) {
        return InitError(_(
                "Disabled transaction index detected.\n\n"
                "Omni Core requires an enabled transaction index. To enable "
                "transaction indexing, please use the \"-txindex\" option as "
                "command line argument or add \"txindex=1\" to your client "
                "configuration file."
            ));
    }

a bit further up (since we can know fTxIndex value well before we look at reindexing just by calling GetBoolArg("-txindex") we should then handle the case correctly.

zathras-crypto commented 9 years ago

Are you a time traveller? ;)

Hehe I think because I based the release on top of the tag, that it uses the tag date.

Some nitpicking,

Let me diff those and see what you're suggesting :)

dexX7 commented 9 years ago

I'll setup a fresh Windows VM in a few minutes, but I'm wondering: what's the relation between our init error and what's going on here?

dexX7 commented 9 years ago

I'm even more surprised a dialog pops up asking to enable the txindex (which is not the one from us).

dexX7 commented 9 years ago

It looks like I missed one of the default values: https://github.com/mastercoin-MSC/mastercore/blob/mscore-0.0.9/src/main.cpp#L3015 https://github.com/bitcoin/bitcoin/compare/0.9...mastercoin-MSC:mscore-0.0.9#diff-7ec3c68a81efff79b6ca22ac1f1eabbaL2997

dexX7 commented 9 years ago

Flipping that value results in the expected situation where the Omni Core "txindex" warning is shown, but when confirming an app crash (BEX64/StackHash) appears nevertheless.

txindex

Edit: might be related: https://github.com/bitcoin/bitcoin/issues/3136

Edit: I'm able to trigger a similar app crash with Bitcoin Core 0.9.3:

  1. Start fresh (remove %APPDATA%\Bitcoin)
  2. Start bitcoin-qt.exe
  3. Shutdown
  4. Start bitcoin-qt.exe -txindex=1
  5. Decline to rebuild index
  6. Crash

Edit: Same issue can be reproduced with Bitcoin Core 0.10.

OS tested: Windows 8.1 x64

zathras-crypto commented 9 years ago

It looks like I missed one of the default values

Shoot, ideally would have caught that in testing but it's difficult without a larger test audience - since we're getting end users testing now I expect a few more of these types of things to crop up.

I'm able to trigger a similar app crash with Bitcoin Core 0.9.3

If we can figure that out perhaps we can push upstream.

I think we'll see what crops up over the coming days and squeeze some minor fixes into a 0.0.9.2 branch without a huge amount of work.

zathras-crypto commented 9 years ago

If you want to PR your fix here https://github.com/zathras-crypto/mastercore/tree/0.0.9.2-Z I'll merge it.

Thanks Z

dexX7 commented 9 years ago

Pushed.

Unfortunally it looks like the error goes deeper:

[EventType BEX] Indicates a buffer overflow (/GS) or DEP exception (BEX64 indicates a buffer overflow (/GS) or DEP exception on 64-bit versions of Windows)

However, when forcefully disabling the Data Execution Prevention/DEP via bcdedit, the errors still pops up, so it looks like it's related to a buffer overflow. This was further confirmed by running ProcMon.

https://technet.microsoft.com/en-us/library/cc738483(v=ws.10).aspx https://msdn.microsoft.com/en-us/library/aa290051(v=vs.71).aspx

bufferoverflow

dexX7 commented 9 years ago

It seems somehow be related to:

==7350== 160 bytes in 1 blocks are definitely lost in loss record 9 of 16
==7350==    at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7350==    by 0x58155D4: __os_malloc (in /usr/lib/libdb_cxx-4.8.so)
==7350==    by 0x57E406E: __env_alloc (in /usr/lib/libdb_cxx-4.8.so)
==7350==    by 0x57A3F44: __lock_open (in /usr/lib/libdb_cxx-4.8.so)
==7350==    by 0x57EA4A7: __env_attach_regions (in /usr/lib/libdb_cxx-4.8.so)
==7350==    by 0x57EA5C1: __env_open (in /usr/lib/libdb_cxx-4.8.so)
==7350==    by 0x570A969: DbEnv::open(char const*, unsigned int, int) (in /usr/lib/libdb_cxx-4.8.so)
==7350==    by 0x353C93: CDBEnv::Open(boost::filesystem::path const&) (db.cpp:101)
==7350==    by 0x15A326: AppInit2(boost::thread_group&) (init.cpp:838)
==7350==    by 0x1425A7: AppInit(int, char**) (bitcoind.cpp:146)
==7350==    by 0x1385CE: main (bitcoind.cpp:175)

In particular and to confirm: when using -disablewallet the DB stuff is not used and it works flawless on Windows.

Edit: just noticed my master was outdated.

It might be fixed here already: https://github.com/bitcoin/bitcoin/pull/5852/files, but I'm still building (and it's probably going to take a while for the Win build).

dexX7 commented 9 years ago

I located the problem and pushed a PR upstream: https://github.com/dexX7/bitcoin/commit/317e66c741aef0fd272e50aa2e82ff192ca5f7e5

You're welcome to test it. :)

dexX7 commented 9 years ago

Re: txindex crash

Fix was merged upstream: https://github.com/bitcoin/bitcoin/pull/5877

zathras-crypto commented 9 years ago

Nicely done mate, nicely done :)

dexX7 commented 9 years ago

This should be converted into a doc at some point, but given the release was done, this can be closed in my opinion.