dimxy / komodo Goto Github PK

I'm not sure this is a practical issue, but it's possible to add kogs to containers that do not belong to you.

This can be reproduced by using kogsaddkogdstocontainer with a kog you own and a container you do not own.

Reordered checks in CheckTokensvout: SpkHasEvalcodeCCV2 call moved topmost, so it would not mistakenly try to check non tokens vouts as tokens'
also bad opreturn decode returned -1 instead 0 (would fail cc tx with bad token opreturns)

A recent change( either f5b0be6 or 73a7829 can find out which if needed) has broken all games.

After doing kogscommitrandoms, kogsrevealrandoms, kogsdeposittokens and kogscreatefirstbaton, the game will not be able to progress further. Both players will see:

{
  "result": "error",
  "error": "could not find baton for your pubkey (not your turn)"
}

while trying to do kogsslamdata

The kogsobjectinfo output for this game object:

{
  "result": "success",
  "evalcode": "f4",
  "objectType": "G",
  "objectDesc": "KOGSID_GAME",
  "version": "1",
  "nameId": "",
  "descriptionId": "",
  "originatorPubKey": "031fed192588f1ce03ec398705576083fac76f6e8f37bc0546e3f79fdbe43fc4d7",
  "creationtxid": "e72143ab6e7e4da77b755dd61e374dd18ace03fe6298c26b4f848681744e3c38",
  "gameinfo": {
    "error": "can't load baton"
  }
}

In first implementations any node created batons.
As we use baton in mempool (to make games responsive)
it is a problem if several nodes created batons. The problem is that first player turn is determined randomly, so if two nodes created batons with different player turns it was possible two baton-slam chains grew in mempool.
To prevent this, now the game creator could only create the first baton.
Then slam tx also sends the baton to next player in turn

now assetid in the rpc param is not checked for assetid in order opreturn

this occured on test DIMXY11 kogs chain run with renidex
failed on no inputs assert in miner.cpp
if (fDebug) assert("mempool transaction missing input" == 0);
this happened only once, on the second try -reindex the error did not reappear

code in CheckTokensV2CreateTx has duplication with TokenExactAmounts and TokenExactAmounts turns out to make only pertial checks
better to get rid of CheckTokensV2CreateTx and have TokenExactAmounts with combined checks

Now if node could not add tx to mempool from another node immediately bans that node
But for cc it might be a normal situation if a cc tx could not be added to mempool as some needed cc tx might be missed (for example if the node was not running that time yet)

It should not ban nodes if cc tx failed to add to mempool

The kogsgameconfiglist rpc output can be spammed by keys other than the system key.

I see there is a check here that attempts to prevent this; however this is simply checking whether the pubkey in the transaction's OP_RETURN output has the system key in it. This data in the OP_RETURN can be changed arbitrarily and allows any node to add entries to kogsgameconfiglist's output.

do final call from Unity

to make identical behaviour with SetCCunspents that does not check if the utxo is spent in mempool
(so such check must be done outside)

As the title says, adding creationtxid to a baton's enclosure object can cause a games containers and slammer tokens to be unrecoverable. It seems these games are not able to be autofinished by the syspk or by the players of the game.

This can be reproduced by adding this line to kogs.cpp, creating a game from this node and attempting to play the game(kogsstartgame, kogscommitrandoms, kogsrevealrandoms, kogscreatefirstbaton) until it fails at kogsslamdata.

enc.creationtxid = Parseuint256("deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef");

This same method of adding creationtxid to enclosures that should not have it can be used to add invalid game ids to a player's game list by creating a kogsstartgame transaction with creationtxid. This is likely to break GUIs if they assume all txids in kogsgamelist response are valid game objects.

I suspect there are more instances of this causing issues. A good solution to this may be to implement CCv2, so that each opreturn can be fully validated.

async broadcast in libnspv,
require requestid into nspv protocol (or maybe REST requestid could be used?)

see how non-mempool unspent Index doing that

in the fillask/bid rpc check if the order is not already spent.
Also if it is spent check if the spent tx is in the active chain (not in an orphan block)

there is a mempool.remove in RunCCEval mentioning airdrop problem
this stmt actually hangs komodod bcz of dead lock (in mempool)
looks like a hack and should be removed

Mempool txns might be reordered (but direct tx vin to prev tx vout references are preserved)
But mempool module is not aware of cc tx to tx indirect dependences and might reorder them in a bad way (as an example of such dependency, it could be that for a kogs baton tx it is checked that container txns are sent to the game address).

Currently existing such problems are solved in a way that txns sending containers to the game addr should be mined in a block.
But we want to work with mempool txns too, to make games more responsive and for this it is needed that

except tokens v2

now it is possible if askamount or bidamount is too low then unit_price may be 0

now if in tokenfillask fillunits > available units, a invalid txns is created.
Better to check SetAskFillamounts result and return error (same for SetBidFillamounts

Started chain with params:
./komodod -ac_name=PRICEST -ac_cc=2 -ac_supply=100000000 -ac_reward=1000000000 -ac_cbopret=7 -ac_prices="LTC, BCHABC, XMR, IOTA, ZEC, WAVES, LSK, DCR, RVN, DASH, XEM, BTS, ICX, HOT, STEEM, ENJ, STRAT" -ac_stocks="AAPL, IBM, EA" -ac_snapshot=50 -ac_sapling=1 -ac_earlytxidcontract=237 -daemon

Then when I'm starting mining:
./komodo-cli -ac_name=PRICEST setgenerate true 1
daemon deadly hangs.

If remove -ac_stocks param chain mines fine

as a result deposit tx with 0 txfee created;
also TokenBeginTransferTx did not allow mempool inputs

This was because CAddrMan::Select running in parallel block addrman cs for a long time (several min).
so getaddr cmd might wait for LOCK(cs) for that long time.
Changed LOCK(cs) to TRY_LOCK(cs) in GetAddr() so it never waits for unlock

should try to reconnect if no nodes

move srang to AppInit; also check how init random for default_random_engine

check for duplication and add appearanceIdto resolve

It's possible to add additional outputs to a tokenv2create transaction resulting in tokenv2list being spammed. I added about 100k entries on the TKLLIVE chain. The rpc is now unusable.

 time komodo-cli -ac_name=TKLLIVE tokenv2list | jq length
104076
komodo-cli -ac_name=TKLLIVE tokenv2list  0.20s user 0.04s system 0% cpu 3:13.11 total
jq length  0.15s user 0.01s system 0% cpu 3:13.12 total

Now coinbases are added as normal inputs to mtx for any confirmations
this might lead to AcceptToMempool missing inputs errors (if reorgs)
Use maturity

this occurs sometime in the Unity test game
on restarting the test with the same gameid the test usually resumes normally

The readme mentions a -wsport= startup param. This does not function as described.

Start a node as : ./komodod -ac_name=WS -ac_supply=999999 -ac_reward=100000000000 -ac_cc=2 -wsport=8888 -testnode=1

It listens instead on the default port 8192.

Didn't look at the code. Will dig deeper sometime in the next few days if you do not .

This issue appeared in token-fix-v2 branch.
When loading 100K cc txns (with PoW mining turned on on the node) at the moment of block creation a -25 error occurred in the sendrawtransaction call.
Surrounded TestBlockValidity with cs_main and the error cleared.
Then optimised myIsutxo_spentinmempool with the use of mempool spent index - the err reappeared.
Added CMempoolStateSaver - the err continued
Used myAddToMempool with mempool indexes fixes (instead of simply mempool.addunchecked ) in CheckBlock() - the err cleared

suspected that mempool sometimes drops good txns

There is no validation on where the "turn baton" output must be sent to for kogsslamdata.

If a malicious party changes this CC address, it will make the deposited tokens of the game unrecoverable. While attempting to recover deposited tokens from a game like this, you will receive the following error:
CC Eval EVAL_KOGS Error!: invalid oper with game: could not add or remove tokens while game is running spending tx bdf0173c570c640c66e03d7c0c800beec9b5708bcbc7b9640d52fd66c1233f47

This leads to both the slammers and the containers being entirely unrecoverable.

To reproduce this, change kogsPk and/or destpk in the following line prior to doing kogsslamdata.

An example of this can be found on the MYKOG chain with gameid, 5951db41a86d096fbb27179b33259b6212e83cbb861515357b5f7ab764938f19. This txid:c70f9ab6fea922ac6c36a4d689f99b74acaa1912d3d805293d84e274722b8cc8 is the malformed kogsslamdata transaction.

use mempool spent Index

to allow spv nodes load balancing

Game ids will disappear from the kogsgamelist my result after one player does kogscommitrandoms.

It would be useful if kogsgamelist could be split up into three categories: not started, in progress and finished.

It seems that the function FindAdvertisings is meant to filter out any advertising batons that were not created by the playerid's originatorPubKey. However, this doesn't seem to work as intended and anyone can create a seemingly valid advertising transaction for any playerid. I understand that only PLAY_FOR_FUN is implemented at the moment, but I believe this will lead to issues once the other ops are implemented.

This line in particular seems to try to filter out advertising transactions that do not include an input from the originatorPubKey, but I believe this may be checking against the playerid creation transaction rather than the advertising transaction.

This can be reproduced by commenting this if statement and using the kogsadvertiseplayer rpc with a playerid of another player.

A similar method can be used to create duplicate entries in the kogsadvertisedplayerlist. Simply comment this if statement and use the kogsadvertiseplayer rpc with a playerid already in the list.

I've not tested the effects of the duplicate entries on game logic. Although it does seem that it will use the first advertise transaction it finds from the marker address.

This issue can likely be solved with CCv2 and adding validation of kogsadvertiseplayer OP_RETURNs.

Noticed that a komodod node tries to reconnect an unknown node ip-address multiple times. That unknown node apparently was probing the komodod port. Need to study how this happens

to make hard link between the baton and the deposit container and slammer tx. This would prevent the game getting into an invalid state if the deposit tx is lost as the result of a reorg

(should the validation code check this reference? maybe not as it is only technological, does not influence tx security)

see how non-mempool addressindex calcs this in main.cpp

Once I watched a qa token_test failure:

making valid tokenv2bid to test cancelbid...

tx is in mempool still probably, let's wait a little bit more
Error:  'confirmations'
AcceptToMemoryPool missing inputs for tx 8da33a1ffbd47c71d109e53abdf7890142fac39e8e5cf4cc572f123e5a47d0ee prevout.hash=e1519155f7e0dc7df3dd492915eda5bbff993f9e47ee68893aa9faa028cc3e07

Probably again mempool issues.
Maybe this is normal if the utxo has been already spent

This error occurs on a mining node with cc enabled.
Messages 'ERROR: mempool transaction missing input' are recorded in node's debug.log.
There are transactions that stay in mempool although they are added into a block too (they are the reason of such messages as their inputs are already spent by the same txns in mempool.

The reason for such errors are incorrectly cleared transactions in mempool for asset chains.

In more details:
For assets chains cc transaction might load other transactions in the same block (with myGetTransaction function)
To allow myGetTransaction to load txns from the same block a special processing is implemented for connected blocks: all txns are temporarily sent into local node mempool (no further propagation to other nodes).
Normally at the end of validation those txns are removed from the mempool when the block validation is finished and the block is added to the chain tip.
But in a case when the block validation is failed the txns are not cleared from the mempool due to insufficient processing for such case. On a mining node, if two or more threads are mining, some threads might normally fail block PoW mining and this issue might appear. So if one mining thread succeeded and other failed it might happened that block txns simultaneously are added in the block and stay in mempool.

This should be fixed by more accurate mempool cleanup.

(this issue has the same cause as #41)

saw on macos asset dd_lock invalid parameter
Sure this is because of lock initialisation/destruction concurrency in static init and CAddrMan::Clear() called in static init constructor.
Suggestion to call Clear() in a dedicated CAddrMan::Init()

CCchange var maybe not inited

increase the libnspv timeout

f5b0be6 breaks the kogsadvertiseplayer rpc command.

komodo-cli -ac_name=KT kogsadvertiseplayer '{"playerid": "7076954b7d2503a1631adf0620185a369e9161b9c2be8ea20b5815c6cbcf50a2", "opts": ["playforfun"]}' { "result": "error", "error": "can't load player object" }

2020-07-19 05:31:55 kogs DecodeGameObjectOpreturn could not find token vouts in txid=7076954b7d2503a1631adf0620185a369e9161b9c2be8ea20b5815c6cbcf50a2

There was a situation when running a test game script in Unity,
when a call to kogsobjectinfo returned two batons but the next call returned only one baton (and the test run failed because in fact it was not correct and there was 3 batons. A yet another call to kogobjectinfo showed the correct 3 batons )
Maybe to do a retry of kogsobjectinfo (or to do a retry inside the function CCgetspenttxid)?

Describe the issue

The daemon crashes when trying to create an NFT without any NFT data in the RPC code

Can you reliably reproduce the issue?

Yes, just try and create an NFT without any data

Expected behaviour

An error code should populate saying data is a required field. Maybe even mention the arbitrary data evalcode 00

The version of Komodo you were using:

test-eval-nft branch

Any extra information that might be useful in the debugging process.

./komodo-cli -ac_name=TKLTEST tokenv2create NONFTDATA1 0.00000001 "This NFT is created to test when no NFT data is added"

(make sure server is running and you are connecting to the correct RPC port)```

It throws this error code and crashes the daemon.