Treat Claim transactions as the highest low priority transactions + Unit test #565

jsolman · 2019-01-19T02:06:57Z

In order to ensure claims transactions get processed with a large backlog of low priority transactions, we should sort the claim transactions first among the low priority transactions.

igormcoelho · 2019-01-19T02:10:04Z

I think this is related to neo-project/neo-modules#39, because we have two options here:
(i) access plugin parameters to allow Mempool behave as expected on a future selection
(ii) hardcode Claim with higher priority than other "free" tx
I really don't know which option is better... any thoughts @shargon , @erikzhang ?

jsolman · 2019-01-19T02:14:35Z

We can expose the higher priority transaction types from the plugin instead of hardcoding claim here if you want, but I think it is find to hardcode claim as the highest of the low priority transactions from the perspective of the MemoryPool.

jsolman · 2019-01-19T02:15:58Z

It should be fine to accept option ii, because the plugin can still decide not to include the claim transactions if it wants to treat them lower priority. Also more low priority transactions will get verified during Blockchain idle.

shargon · 2019-01-19T10:43:49Z

Looks fine for me, we an deal with this special cases here.
Here is like a low priority, but inside of this pack, is hight prioritized. Otherwise is treaded as hight priority (more than 20 tx per block) right @jsolman ?

vncoelho · 2019-01-19T12:27:35Z

neo/Ledger/PoolItem.cs

+            {
+                bool thisIsClaimTx = Tx is ClaimTransaction;
+                bool otherIsClaimTx = otherTx is ClaimTransaction;
+                if (thisIsClaimTx != otherIsClaimTx)


simple and precise, Jeff.

vncoelho · 2019-01-19T12:29:58Z

@shargon, here in the mempool the free txs have not "guarantee" of having their 20txs slot.
I was thinking about this yesterday.
If capacity is full the only metric used to remove is the attached net fee.

Do you think that it is a problem, @shargon?

jsolman · 2019-01-19T17:34:14Z

@vncoelho I also thought about that. If we decide to reserve space in the MemoryPool for low priority transactions we should probably reserve 5% of the pool for low priority transactions or something like that, but actually doing so doesn’t really seem like it matters much to me, and I’ll explain why below.

It would cost someone a lot to keep the pool full of high priority transactions. Adding 50000 high priority transactions costs 50 gas, and it would cost .5 gas every block, there after to keep it full. However this increases to even more costs if we upped how many tx could be accepted each block, and with that many fees in the pool the CN nodes would probably do this.

Also eventually someone that really wants to do that could also be spamming low priority tx with a fee of .00099999 and low tax hashes, and it would have the same effect of requiring everyone who wants to send something to include a fee. I don’t really think it is an issue to reserve space in the pool for low priority tx for this reason.

vncoelho · 2019-01-19T17:49:17Z

Great insight and line of reasoning, Jeff!
The only point I see is that the 50 GAS are not really wasted if transaction expired.
Anyway, I think that with the current design the new mempool mechanism is going to behave smooth and nice. Congratulations!

neo/Ledger/PoolItem.cs

igormcoelho · 2019-01-19T18:33:03Z

Nice and simple change Jeff! Do you think Mempool is following exactly the same logic as plugin filtering now?

jsolman · 2019-01-20T03:26:46Z

@igormcoelho yes, the logic appears to be the same as the plugin now.

jsolman · 2019-01-20T07:09:19Z

We may still want to reserve some percentage of the space in the MemoryPool for low priority transactions so claim transactions can’t be denied during a full pool of high priority transactions.

igormcoelho · 2019-01-20T18:03:37Z

In fact, we shouldn't rely very much only on mempool logic, because each client can (and will) implement a different logic (neo-python, neo-go, neo-js, ...). Another point is that mempool shouldn't get to 50k transactions, unless when actually needed! So, we should enforce validations that guarantee that: no double-spending is happening on same tx on mempool (I don't know if this is tested), and this could be done by blocking UTXO input reference as soon as tx enters, and unblock it as soon as tx leaves (on block relay, it will be blocked permanently). Other verifications could be harder, as they could involve storage and order-dependent tx processing, which are only definitive after block persist.
Anyway, this is another great step, congratulations Jeff for pushing this through.

* Handles escape characters in JSON * Pass ApplicationExecution to IPersistencePlugin (neo-project#531) * Update dependencies: (neo-project#532) - Akka 1.3.11 - Microsoft.AspNetCore.ResponseCompression 2.2.0 - Microsoft.AspNetCore.Server.Kestrel 2.2.0 - Microsoft.AspNetCore.Server.Kestrel.Https 2.2.0 - Microsoft.AspNetCore.WebSockets 2.2.0 - Microsoft.EntityFrameworkCore.Sqlite 2.2.0 - Microsoft.Extensions.Configuration.Json 2.2.0 * change version to v2.9.4 * Updating Unknown to Policy Fail (neo-project#533) * Fix a dead lock in `WalletIndexer` * Downgrade Sqlite to 2.1.4 (neo-project#535) * RPC call gettransactionheight (neo-project#541) * getrawtransactionheight Nowadays two calls are need to get a transaction height, `getrawtransaction` with `verbose` and then use the `blockhash`. Other option is to use `confirmations`, but it can be misleading. * Minnor fix * Shargon's tip * modified * Allow to use the wallet inside a RPC plugin (neo-project#536) * Improve Large MemoryPool Performance - Sort + intelligent TX reverification (neo-project#500) Improve Large MemoryPool Performance - Sort + intelligent TX reverification (neo-project#500) * Keep both verified and unverified (previously verified) transactions in sorted trees so ejecting transactions above the pool size is a low latency operation. * Re-verify unverified transactions when Blockchain actor is idle. * Don't re-verify transactions needlessly when not at the tip of the chain. * Support passing a flag to `getrawmempool` to retrieve both verified and unverified TX hashes. * Support MaxTransactionsPerBlock and MaxFreeTransactionsPerBlock from Policy plugins. * Rebroadcast re-verified transactions if it has been a while since last broadcast (high priority transactions are rebroadcast more frequently than low priority transactions. * Policy filter GetRelayResult message (neo-project#543) * Policy filter GetRelayResult message * adding fixed numbering for return codes * Removed enum fixed values * Add some initial MemoryPool unit tests. Fix bug when Persisting the GenesisBlock (neo-project#549) * More MemoryPool Unit Tests. Improve Re-broadcast back-off to an increasing linear formula. (neo-project#554) * Ensuring Object Reference check of SortedSets for speed-up (neo-project#557) * Minor comments update on Mempool class (neo-project#556) * Update MemoryPool Unit Test to add random fees to Mock Transactions (neo-project#558) * Add Unit Test for MemoryPool sort order. Fixed sort order to return descending. (neo-project#559) * Add unit test to verify memory pool sort order and reverification order. Fixed sort order bug. * VerifyCanTransactionFitInPool works as intended. Also inadvertantly verified GetLowestFeeTransaction() works. * Benchmark structure for UInt classes (neo-project#553) * basic benchmark structure for UInt classes * commented code2 from lights for now * updated tests. all seem correct now * Switch to using a benchmark method taking a method delegate to benchmark. * Make pass. * 1 million iterations. * Switch to ulong for the 4th option, and it is still the same speed. * fix test data for 50% equal data * make test pass * neo.UnitTests/UT_UIntBenchmarks.cs * neo.UnitTests/UT_UIntBenchmarks.cs * Base 20 - UInt160 tests * neo.UnitTests/UT_UIntBenchmarks.cs * inlined 160 * complete tests with UInt256 and UInt160 * neo.UnitTests/UT_UIntBenchmarks.cs * Lights division calculation * Treat lower hashes as higher priority. Fix MemoryPool UT for Hash order. (neo-project#563) * Treat lower hashes as higher priority. * Fix MemoryPool UT for Hash order. * Renaming Trasanction in PoolItem for clarity. * Make PoolItem independent and add PoolItem tests (neo-project#562) * make poolitem independent * Merging * Multiply by -1 * Fix other * Fix Tx * Removing -1 extra multiplication * Fix * make PoolItem internal and added test class * Update PoolItem.cs * added comments for PoolItem variables * getting time from TimeProvider to allow testing * basic test * reset time provider * Add Hash comparison * Adding time provider again and equals * Fix arithmetic * Comment on PoolItem * Update PoolItem.cs * protecting tests against TimeProvider changes on fails * reusing setup part * fixed serialization properties * Improve generation of creating mock DateTime values. Implement hash comparison tests. * Adjust comment. * Treat Claim transactions as the highest low priority transactions. (neo-project#565) * Allow persistence plugins to commit as a final step. (neo-project#568) * Allow persistence plugins to commit as a final step. * Plugins commit before core commits, once all plugins have handled initial work OnPersist. * Allow PersistencePlugin to determine whether to crash if commit fails. * Add ShouldThrowExceptionFromCommit method to IPersistencePlugin. * Throw all commit exceptions that should be thrown in an AggregateException. * Add a Plugin type for observing transactions added or removed from the MemoryPool. (neo-project#580) * Correctly handle conversions between JSON objects (neo-project#586) * Fix neo-project/neo-node#297 (neo-project#587) * Replace new JArray with .ToArray (AccountState) (neo-project#581) * Ensure `LocalNode` to be stoped before shutting down the `NeoSystem`

…eo-project#565)

Add an detail explanation for getapplicationlog api.

Treat Claim transactions as the highest low priority transactions.

368f073

jsolman requested a review from igormcoelho January 19, 2019 02:07

igormcoelho requested review from erikzhang and shargon January 19, 2019 02:08

shargon approved these changes Jan 19, 2019

View reviewed changes

vncoelho approved these changes Jan 19, 2019

View reviewed changes

igormcoelho reviewed Jan 19, 2019

View reviewed changes

neo/Ledger/PoolItem.cs Show resolved Hide resolved

igormcoelho approved these changes Jan 19, 2019

View reviewed changes

jsolman merged commit 7f6e552 into master Jan 20, 2019

erikzhang deleted the MakeClaimTxHigherLowPriority branch January 20, 2019 14:24

txhsl mentioned this pull request Feb 25, 2019

Update from master branch txhsl/neo#2

Merged

rodoufu pushed a commit to rodoufu/neo that referenced this pull request Mar 5, 2019

Treat Claim transactions as the highest low priority transactions. (n…

518a59e

…eo-project#565)

Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020

Update v2.7.6.md (neo-project#565)

2e6d27a

Add an detail explanation for getapplicationlog api.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Treat Claim transactions as the highest low priority transactions + Unit test #565

Treat Claim transactions as the highest low priority transactions + Unit test #565

jsolman commented Jan 19, 2019

igormcoelho commented Jan 19, 2019

jsolman commented Jan 19, 2019 •

edited

Loading

jsolman commented Jan 19, 2019 •

edited

Loading

shargon commented Jan 19, 2019 •

edited

Loading

vncoelho Jan 19, 2019

vncoelho commented Jan 19, 2019 •

edited

Loading

jsolman commented Jan 19, 2019 •

edited

Loading

vncoelho commented Jan 19, 2019

igormcoelho commented Jan 19, 2019 •

edited

Loading

jsolman commented Jan 20, 2019

jsolman commented Jan 20, 2019 •

edited

Loading

igormcoelho commented Jan 20, 2019

Treat Claim transactions as the highest low priority transactions + Unit test #565

Treat Claim transactions as the highest low priority transactions + Unit test #565

Conversation

jsolman commented Jan 19, 2019

igormcoelho commented Jan 19, 2019

jsolman commented Jan 19, 2019 • edited Loading

jsolman commented Jan 19, 2019 • edited Loading

shargon commented Jan 19, 2019 • edited Loading

vncoelho Jan 19, 2019

Choose a reason for hiding this comment

vncoelho commented Jan 19, 2019 • edited Loading

jsolman commented Jan 19, 2019 • edited Loading

vncoelho commented Jan 19, 2019

igormcoelho commented Jan 19, 2019 • edited Loading

jsolman commented Jan 20, 2019

jsolman commented Jan 20, 2019 • edited Loading

igormcoelho commented Jan 20, 2019

jsolman commented Jan 19, 2019 •

edited

Loading

jsolman commented Jan 19, 2019 •

edited

Loading

shargon commented Jan 19, 2019 •

edited

Loading

vncoelho commented Jan 19, 2019 •

edited

Loading

jsolman commented Jan 19, 2019 •

edited

Loading

igormcoelho commented Jan 19, 2019 •

edited

Loading

jsolman commented Jan 20, 2019 •

edited

Loading