feat(core-transaction-pool): Switch expiration to chain height #2461

vasild · 2019-04-23T15:33:59Z

Switch from calculating transactions' expiration in "number of seconds
since genesis block" to "chain height".

Notice: this change alters the semantic of the maxTransactionAge beyond
just the units (seconds vs height). Before the change we would use the
user-defined transaction.timestamp and add maxTransactionAge (seconds)
to it. After the change we use the current chain height when we have
received the transaction (which is not user-defined and not necessary
the same as when the transaction was created by the user) and add
maxTransactionAge (height) to it.

Resolves #1060

Summary

What kind of change does this PR introduce?

Does this PR introduce a breaking change?

Yes
No

Checklist

I have read the CONTRIBUTING documentation
Lint and unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Other information

Switch from calculating transactions' expiration in "number of seconds since genesis block" to "chain height". Notice: this change alters the semantic of the maxTransactionAge beyond just the units (seconds vs height). Before the change we would use the user-defined transaction.timestamp and add maxTransactionAge (seconds) to it. After the change we use the current chain height when we have received the transaction (which is not user-defined and not necessary the same as when the transaction was created by the user) and add maxTransactionAge (height) to it. Resolves #1060

ghost · 2019-04-23T15:34:50Z

The ci/circleci: test-node10-e2e job is failing as of 21845ee79cfe1c89f0331691453abe84a94adfce. Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

faustbrian · 2019-04-23T15:36:46Z

packages/core-transaction-pool/src/defaults.ts

+    // Max transaction age in number of blocks produced since receiving a transaction.
+    // If a transaction stays that long in the pool without being included in any block,
+    // then it will be removed.
+    maxTransactionAge: 2700,


Rename this to something better so the comment is not needed to understand what it does.

I think the variable name is fine, any suggestions are welcome though.

It is not possible to condense the information from 3 lines into a variable name. In this case the important information the comment is conveying is:

The units of measurement (it is blocks, not seconds)

When does the counter start ticking (it is when the transaction has been received, not when it was created like it was before this change)

What happens when the time expires.

Please, do not remove the comment.

It is normal to have one or two sentences describing each variable/config parameter, for example:

-debuglogfile=<file> Specify location of debug log file. Relative paths will be prefixed by a net-specific datadir location. (-nodebuglogfile to disable; default: debug.log) -includeconf=<file> Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line) -loadblock=<file> Imports blocks from external blk000??.dat file on startup -maxmempool=<n> Keep the transaction memory pool below <n> megabytes (default: 300) -maxorphantx=<n> Keep at most <n> unconnectable transactions in memory (default: 100) -mempoolexpiry=<n> Do not keep transactions in the mempool longer than <n> hours (default: 336) -par=<n> Set the number of script verification threads (-8 to 16, 0 = auto, <0 = leave that many cores free, default: 0) -persistmempool Whether to save the mempool on shutdown and load on restart (default: 1) -prune=<n> Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex and -rescan. Warning: Reverting this setting requires re-downloading the entire blockchain. (default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=550 = automatically prune block files to stay under the specified target size in MiB)

How is it not possible to convey that minuscule bit of information in a variable name? All of the following make it pretty clear that it is about lifetime/expiration of the transaction and the unit of measurement is a number of blocks.

transactionTtlInBlocks

transactionLifetimeInBlocks

transactionValidForNumberOfBlocks

transactionMaxAgeInBlocks

transactionExpiresAfterNumberOfBlocks

packages/core-transaction-pool/src/memory-transaction.ts

faustbrian · 2019-04-23T15:39:03Z

packages/core-transaction-pool/src/memory.ts

+     * transactions that are without expiration are not included. Used to:
+     * - find all transactions that have expired (have an expiration height
+     *   lower than the current height) - they are at the beginning of the array.
+     */
    private byExpiration: MemoryTransaction[] = [];


Why do you need that long of a comment to understand what an array with the name byExpiration holds? The name is pretty self-explanatory and the code that uses it is also very simple.

If you feel the need to write that long comments, or even comments at all for such trivial things, you should think about why you really need those comments and remove the need for them and figure out how to make the code easier to understand.

The comment is needed to convey the following information, which cannot be deduced from the name of the variable:

The array does not contain all transactions

Is it sorted in ascending or descending order

Imo the information the comment tries to convey is a bit redundant since it can be quickly deduced by checking the relevant code. I personally would remove the comment, too.

I changed this in a followup PR, see if you like it: https://github.com/ArkEcosystem/core/pull/2494/files#diff-ea1f8e8a5fdc70f328cde94d66d99dfeR20

__tests__/unit/core-transaction-pool/connection.test.ts

packages/core-transaction-pool/src/memory.ts

Co-Authored-By: vasild <vd@freebsd.org>

packages/core-transaction-pool/src/memory.ts

and handle the case if we are unable to get it

packages/core-transaction-pool/src/memory.ts

The test needs to query the current chain height and then wait until it increases or fake an increased height.

…tion-expiration-in-height * ArkEcosystem/core/develop: release: 2.3.15 (#2478) release: 2.3.14 (#2477) refactor(core): remove support for old release channels (#2476) fix(crypto): add exceptions for transactions with invalid recipients (#2471) refactor(crypto): make all Slots.* methods static (#2473) docs(template): add release section to pull request template (#2472) chore: fix absolute banner urls (#2469)

packages/core-transaction-pool/src/memory.ts

__tests__/unit/core-transaction-pool/connection.test.ts

…tion-expiration-in-height * ArkEcosystem/core/develop: style: apply linting and formatting to e2e tests (#2480)

codecov-io · 2019-04-25T11:08:43Z

Codecov Report

Merging #2461 into develop will decrease coverage by 0.02%.
The diff coverage is 89.36%.

@@             Coverage Diff             @@
##           develop    #2461      +/-   ##
===========================================
- Coverage    67.79%   67.76%   -0.03%     
===========================================
  Files          386      386              
  Lines         8396     8399       +3     
  Branches       398      399       +1     
===========================================
  Hits          5692     5692              
- Misses        2652     2655       +3     
  Partials        52       52

Impacted Files	Coverage Δ
packages/core-transaction-pool/src/storage.ts	`100% <100%> (ø)`	⬆️
...ore-transaction-pool/src/sequential-transaction.ts	`100% <100%> (ø)`
packages/core-transaction-pool/src/connection.ts	`95.51% <87.5%> (ø)`	⬆️
packages/core-transaction-pool/src/memory.ts	`95.87% <88.88%> (-3.01%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d84842...156cd28. Read the comment docs.

…tion-expiration-in-height * ArkEcosystem/core/develop: feat: add core-state to manage the state of in-memory data (#2479)

packages/core-transaction-pool/src/memory.ts

faustbrian · 2019-04-25T14:58:25Z

packages/core-transaction-pool/src/memory.ts

        }

        return new Set();
    }

-    public remember(MemoryTransaction: MemoryTransaction, maxTransactionAge: number, databaseReady?: boolean): void {
-        const transaction: Interfaces.ITransaction = MemoryTransaction.transaction;
+    public remember(SequentialTransaction: SequentialTransaction, maxTransactionAge: number, databaseReady?: boolean): void {


Suggested change

public remember(SequentialTransaction: SequentialTransaction, maxTransactionAge: number, databaseReady?: boolean): void {

public remember(sequentialTransaction: SequentialTransaction, maxTransactionAge: number, databaseReady?: boolean): void {

Variable names should always be in camelCase.

You named it MemoryTransaction all over the place, I did not wanted to argue and assumed you wanted it to be with capital letter for some reason.

I completely removed the [sS]equentialTransaction from the code in a followup PR: #2494

…tion-expiration-in-height * ArkEcosystem/core/develop: fix(core): parse only the last line of pm2 stdout (#2484) release: 2.3.18 (#2488) fix(core-snapshots): reset unfinished round after import (#2486) fix(core-forger): update configManager height (#2487) release: 2.3.16 (#2483) fix(core-database): initialize height on startup (#2482)

vasild requested review from air1one, faustbrian, kristjank and spkjp as code owners April 23, 2019 15:33

ghost added Complexity: Medium labels Apr 23, 2019

vasild force-pushed the transaction-expiration-in-height branch from 854244a to 537eb5d Compare April 23, 2019 15:40

faustbrian suggested changes Apr 23, 2019

View reviewed changes

ghost added the Status: Needs Changes label Apr 23, 2019

ArkEcosystem deleted a comment Apr 23, 2019

fix(core-transaction-pool): properly get the current chain height

e61fa9f

vasild force-pushed the transaction-expiration-in-height branch from 537eb5d to e61fa9f Compare April 23, 2019 15:42

Merge branch 'develop' into transaction-expiration-in-height

2b76687

faustbrian reviewed Apr 24, 2019

View reviewed changes

packages/core-transaction-pool/src/memory.ts Outdated Show resolved Hide resolved

faustbrian and others added 8 commits April 24, 2019 05:59

Merge branch 'develop' into transaction-expiration-in-height

f6df72c

Update __tests__/unit/core-transaction-pool/connection.test.ts

8186c8a

Co-Authored-By: vasild <vd@freebsd.org>

Update __tests__/unit/core-transaction-pool/connection.test.ts

da79b1c

Co-Authored-By: vasild <vd@freebsd.org>

Update __tests__/unit/core-transaction-pool/connection.test.ts

4675364

Co-Authored-By: vasild <vd@freebsd.org>

Update packages/core-transaction-pool/src/memory.ts

086aa30

Co-Authored-By: vasild <vd@freebsd.org>

chore(core-transaction-pool): rename MemoryTransaction

6c10fe5

chore(core-transaction-pool): get current height from "blockchain"

9794070

chore(core-transaction-pool): remove unused variable

4d1e25c

vasild commented Apr 24, 2019

View reviewed changes

packages/core-transaction-pool/src/memory.ts Outdated Show resolved Hide resolved

faustbrian reviewed Apr 24, 2019

View reviewed changes

packages/core-transaction-pool/src/memory.ts Outdated Show resolved Hide resolved

faustbrian and others added 2 commits April 24, 2019 17:41

Merge branch 'develop' into transaction-expiration-in-height

1e58fe3

fix(core-transaction-pool): get current height in a more robust way

0167421

and handle the case if we are unable to get it

faustbrian reviewed Apr 24, 2019

View reviewed changes

packages/core-transaction-pool/src/memory.ts Outdated Show resolved Hide resolved

chore(core-transaction-pool): remove unreachable code

fb8631b

vasild force-pushed the transaction-expiration-in-height branch from 3241b69 to fb8631b Compare April 24, 2019 16:08

test(core-transaction-pool): disable tx exiration test

31c0835

The test needs to query the current chain height and then wait until it increases or fake an increased height.

faustbrian mentioned this pull request Apr 25, 2019

feat: add core-state to manage the state of in-memory data #2479

Merged

22 tasks

vasild added 2 commits April 25, 2019 10:04

chore(core-transaction-pool): only use app.has if it is available

fd1c4ec

faustbrian reviewed Apr 25, 2019

View reviewed changes

packages/core-transaction-pool/src/memory.ts Outdated Show resolved Hide resolved

faustbrian reviewed Apr 25, 2019

View reviewed changes

__tests__/unit/core-transaction-pool/connection.test.ts Outdated Show resolved Hide resolved

faustbrian reviewed Apr 25, 2019

View reviewed changes

__tests__/unit/core-transaction-pool/connection.test.ts Outdated Show resolved Hide resolved

vasild added 2 commits April 25, 2019 12:55

fix(core-transaction-pool): handle undefined "expiration"

de89d7a

Merge remote-tracking branch 'ArkEcosystem/core/develop' into transac…

68334f7

…tion-expiration-in-height * ArkEcosystem/core/develop: style: apply linting and formatting to e2e tests (#2480)

vasild added 2 commits April 25, 2019 15:42

tests(core-transaction-pool): fix a disabled test by proper mocking

00b0a22

Merge remote-tracking branch 'ArkEcosystem/core/develop' into transac…

21845ee

…tion-expiration-in-height * ArkEcosystem/core/develop: feat: add core-state to manage the state of in-memory data (#2479)

faustbrian reviewed Apr 25, 2019

View reviewed changes

packages/core-transaction-pool/src/memory.ts Show resolved Hide resolved

tests(core-transaction-pool): adjust after a4567cd

e5581c8

spkjp suggested changes Apr 25, 2019

View reviewed changes

packages/core-transaction-pool/src/memory.ts Outdated Show resolved Hide resolved

packages/core-transaction-pool/src/memory.ts Outdated Show resolved Hide resolved

chore(core-transaction-pool): simplify, now that we have the last block

a2b52a0

faustbrian reviewed Apr 25, 2019

View reviewed changes

vasild mentioned this pull request Apr 26, 2019

refactor(core-transaction-pool): simplify by not using insertion order #2494

Closed

22 tasks

faustbrian approved these changes Apr 26, 2019

View reviewed changes

Merge branch 'develop' into transaction-expiration-in-height

156cd28

spkjp self-requested a review April 26, 2019 13:44

spkjp approved these changes Apr 26, 2019

View reviewed changes

faustbrian merged commit 5d48f03 into develop Apr 26, 2019

ghost deleted the transaction-expiration-in-height branch April 26, 2019 14:02

vasild mentioned this pull request Apr 26, 2019

Transaction expiration calculation -> change times to only block time #1060

Closed

ghost mentioned this pull request Apr 28, 2019

[Weekly Digest] Apr 22, 2019 - Apr 28, 2019 #2498

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core-transaction-pool): Switch expiration to chain height #2461

feat(core-transaction-pool): Switch expiration to chain height #2461

vasild commented Apr 23, 2019 •

edited by faustbrian

Loading

ghost commented Apr 23, 2019 •

edited by ghost

Loading

faustbrian Apr 23, 2019

vasild Apr 24, 2019

faustbrian Apr 24, 2019

faustbrian Apr 23, 2019

vasild Apr 24, 2019

spkjp Apr 25, 2019

vasild Apr 26, 2019

codecov-io commented Apr 25, 2019 •

edited

Loading

faustbrian Apr 25, 2019

vasild Apr 25, 2019

vasild Apr 26, 2019

	public remember(SequentialTransaction: SequentialTransaction, maxTransactionAge: number, databaseReady?: boolean): void {
	public remember(sequentialTransaction: SequentialTransaction, maxTransactionAge: number, databaseReady?: boolean): void {

feat(core-transaction-pool): Switch expiration to chain height #2461

feat(core-transaction-pool): Switch expiration to chain height #2461

Conversation

vasild commented Apr 23, 2019 • edited by faustbrian Loading

Summary

What kind of change does this PR introduce?

Does this PR introduce a breaking change?

Checklist

Other information

ghost commented Apr 23, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 25, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasild commented Apr 23, 2019 •

edited by faustbrian

Loading

ghost commented Apr 23, 2019 •

edited by ghost

Loading

codecov-io commented Apr 25, 2019 •

edited

Loading