Add gettradessince_MP

[ghost]

adds rpc call gettradessince_MP, filtered by timestamp, currency1, currency2 if desired

the addition to CMPMetadex is purely for the blocktime, and this could be a way to shorten the method signature by getting access to pblockindex, possibly

there is one FIXME in this PR:

• //FIXME: bug exists where if both currencies are supplied the divisible boolean is flipped

[dexX7]

Actually I think this is sort of redundant. Not the RPC call - which is really nice to have - but storing the block time. There is already a transaction reference, but on top also the block height. An untested alternative:

int64_t getBlockTime() {
  CBlockIndex* pblockindex = chainActive[block];
  return pblockindex->GetBlockTime();  // for unsigned int: pblockindex->nTime
}

https://github.com/mastercoin-MSC/mastercore/blob/mscore-0.0.9/src/main.h#L810

[ghost]

If you read my original comment:

the addition to CMPMetadex is purely for the blocktime, and this could be a way
to shorten the method signature by getting access to pblockindex, possibly

The class needs access to pblockindex, which is a instance of CBlockIndex, before I can use that method: its a possible enhancement for the future, but for now I didn't think it was necessary to do any major surgery.

[dexX7]

the addition to CMPMetadex is purely for the blocktime, and this could be a way to shorten the method signature by getting access to pblockindex, possibly

Sorry, I think I misunderstand the statement you made. Could you please rephrase?

The class needs access to pblockindex ... major surgery.

My assumption was you have access to chainActive and as consequence CBlockIndex* pblockindex = chainActive[block] either directly or indirectly, which is globally set in main.cpp/main.h and used in mastercore.cpp, mastercore_rpc.cpp, mastercore_sp.cpp and mastercore_tx.cpp as well.

[ghost]

My assumption was you have access to chainActive and as consequence CBlockIndex* 
pblockindex = chainActive[block] either directly or indirectly, which is globally set in 
main.cpp/main.h and used in mastercore.cpp, mastercore_rpc.cpp, mastercore_sp.cpp 
and mastercore_tx.cpp as well.

I don't particularly like this method, because it requires allocation of a entire CBlockIndex obj just to get the blockTime, but I can change it to use this if @m21 prefers it

[dexX7]

Yeah, I totally see your point. Iterating the trades, fetching blocks, getting the time, ... on the other hand, but that's just my opinion, adding fields like this introduces an overhead as well. Especially time in Bitcoin isn't really that useful at all imho, given that block times are not necessarily in order and time might as well go backwards.

What's your opinion about using the block height (and maybe also transaction position) instead of time as filter criteria?

[ghost]

It would be simpler I think, and less confusing to look up blockheights instead of timestamps for all comparisons... any objections to that?

[ghost]

On thinking about it, if we had transaction position, another field would have to be added to the instantiator.

No, I think now the best solution is to just pass it pBlockIndex (whenever we get around to it) so that we can derive that piece of info, as well as the blockTime, from the same source and perhaps make either work in the future.

[ghost]

I don't know Dex, the time filter is mainly a feature for integrators, its entirely possible to bypass it by passing a really low number... I think I'd like to hear from (the integrators) directly before I make this change, but maybe some others can weigh in on how simple it would be to get (transaction) location data from the pblockIndex object to trim the fat from the class instantiator... with all the flux in the metadex code structure right now, i think its a strong possibility we can make the change sometime.

[dexX7]

Integrators? Can you provide an example? Please make sure the behavior of Bitcoin time is fully known and understood, if anyone indeed wishes to build a system based on the time. https://bitcoin.org/en/developer-reference#block-header may serve as reference.

I named transaction positions for completeness, but I actually see no use case, but queriny based on height is pretty essential imho. Anyway, in the constructor both values are already given: block height is block and transaction position is idx.

And please don't get me wrong, I'm not suggesting to ditch fetching trades based on time at all, but I'd simply move it somewhere else. You may do the time check in the rpc file. It's not awesome, but trades are already iterated there, too. :)

[ghost]

Not sure this is ready to be merged, Closing until the issues Dexx outlined above are resolved

[ghost]

Bitcoin time is imprecise, but its easier to understand than blocks when you're talking about a specific subset of people - re: integrators, mainly traders or people integrating a front-end for the metadex

There is a use-case for transaction position: more accurate precision of the ordering in the set. If I have 3 orders with the same blockTime, then transaction position can be used for an addional layer of accuracy.

I was considering adding block filtering in addition to timestamp filtration - but only when we have access to both pieces of information (whenever we decide to store pBlockIndex in the future)

[dexX7]

Regarding this PR: I think you should go ahead nevertheless, but with block height instead of time or fetch the time outside of the class. But again, this is just imho. :)

There is a use-case for transaction position: more accurate precision of the ordering in the set.

Totally agree, the position itself is very useful, but I see no use case for it as filter criteria, because transactions arrive in blocks and not in ... half blocks or something, where it might make sense to get only half of the transactions within a block. Bad example.. :)

One might of course use the time to fetch trades, but think of the case where one would constantly fetch trades, e.g. for a charting application: then you'd need to get at least all trades of the last two hours (because that's the fuzzy Bitcoin time window) whenever a new block arrives and then filter out trades that are already known.

Hmm.. I think every transaction type may benefit, if properties such as height, block time or whatever else were available. As far as I see not only CMPMetaDEx has fields like txid, block, but the other transaction classes as well. How about declaring a base class which has all those base properties from which the others are derived?

class CMPBaseTransaction
{
private:
  // Bitcoin specific
  uint256 txid; // or CTransaction, ...
  int height; // or CBlockIndex, uint256 blockhash, ...
  unsigned int position;

  // Mastercoin base properties
  uint16_t version;
  uint16_t type;
  std::string sender; // or CBitcoinAddress, ...
  // ...

public:
  int64_t GetFee();
  // ... GetBlock();
  // ... GetTransaction();
  // ...
}

class CMPCrowd: public CMPBaseTransaction
{
private:
  uint32_t currency_desired;
  uint64_t deadline;
  // ...
}

class CMPOffer: public CMPBaseTransaction
{
private:
  int64_t amount_for_sale;
  uint8_t payment_window;
  // ...
}

// ... etc.

And then do something like:

if (metaTrade.GetBlock().GetBlockTime() >= nTimestamp) {
  // ...
}

... major surgery ...

Hehe. :p

[ghost]

I feel this PR still has merit, for that reason I'm reopening it.

If you'd like to see something changed, why don't you fork my branch (009-add-gettradessince) , make some changes and send me a link? I think that would get things done a lot faster than a discussion, honestly (you can submit PR's to me directly or just list the link, if it looks good i'll pull it in so it can be included here)

Thanks Dexx

[ghost]

Something good did come from the discussion though Dexx - I added the metadex info to gettransaction_MP and fixed the FIXME in the original PR, the gettradessince_MP call should have everything working now... let me know if you see bugs or data issues ! :)

[ghost]

Added implementation (first cut) of gettradehistory_MP

[m21]

Hi Faiz. Sorry wasn't following closely until now.

First, blockTime can be retrieved given the block number at any point in the code. Thus I see no reason carrying it around in various internal classes.

Second, you shouldn't rely on any in-memory structures (metadex, meta, etc.) for historical data; that's why we got the MP_txlist database going. In-memory structures give you the correct state of everything at that point in time (that block height specifically). They contain enough to guarantee the MP-universe balance sheet with additions, but historic data belongs in a separate subsystem (at the extreme end is the block explorer).

P.S. breaking up CMPTransaction into base + children is major surgery :) let it happen at more quiet time.

[ghost]

can someone confirm this behavior?

int64_t getBlockTime() {
  CBlockIndex* pblockindex = chainActive[block];
  return pblockindex->GetBlockTime();  // for unsigned int: pblockindex->nTime
}

I do not get accurate timestamps:

block 299448 and blockTime 1412830132 

 block 299310 and blockTime 1412811193 

 block 299310 and blockTime 1412811193 

 block 299310 and blockTime 1412811193 

 block 299310 and blockTime 1412811193 

 block 299310 and blockTime 1412811193 

 block 299310 and blockTime 1412811193 

 block 272804 and blockTime 1407437167 

[ghost]

my fault, i was thinking in mainnet blocktimes.

i updated the code to not change the method signature, also thanks Dexx, I used your code to get the blocktime.

price information has been commented out while the data structures are in flux

[ghost]

Question about the recent changes @m21, i saw getorderbook commented out (with good reason), will the 'metadex' data structure be going away completely now? I was curious as there doesnt seem to be any sort of 'active' or 'expiry' for old orders... and i saw metadex_delete was also commented out... so there are no more deletes or is that just for the time being?

[dexX7]

Hey Faiz, sorry for the somewhat delayed feedback. I tested your branch pretty much immediately, but I'm sort of unable to tell at the moment, what works and what doesn't. Will try again later, because there is already new stuff added. Two notes on my part (which I need to reconfirm):

• are results in order or out of order?
• does it make a difference, if I use gettradessince_MP XXX 1 3 or gettradessince_MP XXX 3 1?

[dexX7]

Also +1 for add_mdex_fields(), nice job! :)

[ghost]

hey Dexx much appreciated :)

1) results are out of order, for now, i started writing a lexigraphical sort method but had to shift gears as the internal data structures are changing and we need to support storage of the new format so we don't break the RPC moving forward... You'll see getorderbook_MP is disabled because of it at the moment as a result

2) IIRC gettradessince_MP should return both sides of the trade with one currency, but now that I think about it some more testing might need to be done there, the behavior definitely needs to be that way but I'm not sure if it actually does that, yet.

Now that this is merged supporting the new structure is currently priority, but will be coming back for touchups so feel free to drop more comments here if you have them :)

[dexX7]

I agree, it should return both sides and I can imagine this will be tricky until it's intuitive and feels good. Questions that raise: how do you price those? If it's MSC/XLTC on one side, would it be XLTC/MSC the other way around? Etc. etc. :)

If we assume (which I hope doesn't last long) either one side of a pair is MSC, then I suggest we price everything in MSC, even if the currency pair is inverted. I suggest the same for BTC - whenever BTC is involved, then price it in BTC.

While building there are two warnings that arise from this PR, I believe:

mastercore_rpc.cpp:1515:10: warning: ‘valid’ may be used uninitialized
in this function [-Wmaybe-uninitialized]
mastercore_rpc.cpp:1845:9: warning: ‘MPTxTypeInt’ may be used
uninitialized in this function [-Wmaybe-uninitialized]

(Probably with other line numbers)

Quick fix #179