Remove tx 51 v1 from allowed transactions?

[dexX7]

Tx 51, version 1 is not live and not implemented, however the current code technically allows crowdsales with version 1.

If there are no (non test) mainnet crowdsales created with version 1, I think the version number should be set to zero to respect the spec ... well and to avoid that the actual v1 would be v2. ;)

I suggest to address this before the release.

[m21]

@marv-engine comments please?

[marv-engine]

Effective with version 1 of Transaction type 51 and block #(TBD), a single crowdsale is able to accept multiple currencies, including bitcoins (currency id 0), for purchases of a Smart Property in a single crowdsale. ...

How much, if any, of this is implemented now?

[dexX7]

No Bitcoins and only one property accepted, with the calculation formula as provided in the spec. Basically v0 is live, but v1 is accepted during parsing and transaction processing (for reasons I don't know more about).

The relevant question in either case: are there v1 transactions live on mainnet? If so, it's not possible to remove it from the allowed version numbers list, because it would change history. At the same time this would force Master Core to associate v2 with the real v1.

[zathras-crypto]

Weirdness with markdown, comment removed and resubmitted below.

[zathras-crypto]

v1 crowdsales are definitely not live, and should not be live in code. @m21 we do allow packet version 1 in source (MSC_TYPE_CREATE_PROPERTY_VARIABLE, MSC_SP_BLOCK, MP_TX_PKT_V1) - that looks like an error. We should: 1) Do blockchain analysis to ensure no-one has broadcasted and had validated a v1 crowdsale 2) Change to require v0 packets along with 0.0.9 as 0.0.9 breaks consensus anyway and is a hard requirement update

Thanks Z

[marv-engine]

The production Omniwallet database has 67 tx51entries - some valid, some invalid, some Production, some Test (TMSC).

All 67 tx51's are version 0.

There is a new version of the Omniwallet crowdsale page under development that accepts bitcoin and it can also accept multiple currencies. So, for now, that new crowdsale page is ahead of OmniCore, and it shouldn't go into production until OmniCore is ready. @achamely

[dexX7]

Thanks @marv-engine, that's very helpful.

It's a ugly situation, the more I think about it. If the next release disables v1, it creates a similar problem as not providing "enabled at block height xxxx in the future", but instead enabling a new feature in the next release without sufficient time for users to update, which bears the risk of users of not yet updated clients getting exploited.

I do however have an idea: since the release is going to enable "send-to-owners" (right?), which is going to be enabled at some point in the future after a specific block, I suggest to implement the following strategy and to use a dynamic transaction policy:

• Disable tx 51 v1, if there is no v1 on mainnet before STO is allowed
• Otherwise, if within this periode one or more v1 crowdsales appear on mainnet, v1 won't be disabled

In the release after this release, the dynamic handling can be removed and replaced by a hard coded list of allowed transaction types and versions as usual, where v1 is either allowed or disallowed.

[marv-engine]

@dexX7 I think you're bringing up a larger issue - how to add/update/disable transaction functionality in general.

A well thought-out strategy will make things simpler for everyone.

@CraigSellars

[dexX7]

This right now is probably an exceptional situation as it should have never been enabled in the first place, but I fully agree and a well defined release process would be very useful in my opinon.

Roughly, I imagine it should go as follows:

1. A new transaction or feature is proposed
2. Specified
3. Once finalized, a time/block height in the future is defined at which the transaction is enabled
4. Early enough, and before the transaction is enabled, a new Master Core version is published, to ensure users have sufficient time to upgrade
5. At defined time/block height, the transaction gets accepted by Master Core

Inbetween there should be lots of tests, refinements and public announcements.

The alert notification system can play a significant role in this process as well, a) to ensure, really all users receive a message regarding an upcoming update, b) to provide outdated clients with information to prevent any potential loss. This can be in form of "once the transaction is live, shutdown" or "whenever you see transaction xyz, assume the sender has zero tokens from now on" (or something similar). At best, even an outdated client can continue to interact with (parts of) the system.

[m21]

To be decided at the STO tag time.

[m21]

I would be against changing history. (There was original ambiguity in the spec pertaining this, discussed on Skype as well. But that doesn't matter anyway.) I don't see any reason to not accept V1 for this TX type as the code currently allows; the spec can be amended where not clear.

[m21]

Skype vote results: 3 for "no code change & no retest" ; 1 abstaining. V1 is a go (same as V0).

[zathras-crypto]

Agreed, @Craig is going to check in with @JR for a cleanup of the spec and outstanding PR's, and once it's in a decent state I'll PR a quick amendment to state v1 = v0 for crowdsales and push any bitcoin crowdsale stuff into v2 (and future tx section if not already there). Thanks Z

On Tue, Jan 27, 2015 at 9:01 AM, Michael notifications@github.com wrote:

Closed #245 https://github.com/mastercoin-MSC/mastercore/issues/245.

— Reply to this email directly or view it on GitHub https://github.com/mastercoin-MSC/mastercore/issues/245#event-224957258.

[craig]

No, I won't!

[dacoinminster]

I'm here. Are there merge-ready PRs on the spec? Sorry - I didn't think any were quite merge-ready. I'm happy to hit the merge button, but you don't need to wait for me if you guys think something is ready to be merged.

[dacoinminster]

Update: I looked over the pull requests. The most recently updated pull request was touched in November. I think it's safe to do a PR against the live version without worrying about the others for now.