Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(core-transaction-pool): segregate pools by wallets #3499

Draft
wants to merge 12 commits into
base: 3.0
from

Conversation

@rainydio
Copy link
Contributor

rainydio commented Feb 13, 2020

Summary

Each sender gets his own pool state and cannot interfere with others.

Checklist

  • Documentation
  • Ready to be merged
@faustbrian faustbrian changed the title feature(transaction-pool): segregated pool refactor(transaction-pool): segregated pool Feb 13, 2020
@faustbrian faustbrian changed the title refactor(transaction-pool): segregated pool refactor(core-transaction-pool): segregate pools by wallets Feb 13, 2020
@faustbrian faustbrian self-requested a review Feb 13, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #3499 into 3.0 will increase coverage by 0.62%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##              3.0    #3499      +/-   ##
==========================================
+ Coverage   47.37%   48.00%   +0.62%     
==========================================
  Files         538      538              
  Lines       13536    13362     -174     
  Branches     1823     1794      -29     
==========================================
+ Hits         6413     6414       +1     
+ Misses       7094     6919     -175     
  Partials       29       29              
Impacted Files Coverage Δ
packages/core-transaction-pool/src/memory.ts 0.00% <0.00%> (ø) ⬆️
...ackages/core-transactions/src/handlers/one/vote.ts 0.00% <0.00%> (ø) ⬆️
...ackages/core-transactions/src/handlers/two/ipfs.ts 0.00% <0.00%> (ø) ⬆️
...ckages/core-p2p/src/socket-server/versions/peer.ts 0.00% <0.00%> (ø) ⬆️
...ages/core-transaction-pool/src/service-provider.ts 0.00% <0.00%> (ø) ⬆️
...ages/core-transactions/src/handlers/transaction.ts 0.00% <0.00%> (ø) ⬆️
...ges/core-transactions/src/handlers/one/transfer.ts 0.00% <0.00%> (ø) ⬆️
...es/core-transactions/src/handlers/two/htlc-lock.ts 0.00% <0.00%> (ø) ⬆️
...s/core-transactions/src/handlers/two/htlc-claim.ts 0.00% <0.00%> (ø) ⬆️
.../core-transactions/src/handlers/two/htlc-refund.ts 0.00% <0.00%> (ø) ⬆️
... and 16 more

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 acf3423...82584cb. Read the comment docs.

@faustbrian

This comment has been minimized.

Copy link
Collaborator

faustbrian commented Feb 14, 2020

Will go over this on monday.

@faustbrian

This comment has been minimized.

Copy link
Collaborator

faustbrian commented Feb 14, 2020

@rainydio merge conflicts

Copy link
Collaborator

faustbrian left a comment

I left some initial (mostly cosmetic/semantic) remarks but there are a few more things in general.

Types

Always add types to variables, functions and return values. I know that TypeScript and an IDE can just show you all types but the code should reveal its intent, structure and types without needing an IDE to see what is what or having to jump around 5 files to see what a function is returning.

If someone is using vim, emacs or some barebones editor like sublime text they will need to dig through the code to see that method X from another package is returning an object of a specific type because the use of the code only was const var = class.method(); instead of const var: ObjectType = class.method();.

This might seem redundant at first glance but goes a long way in terms of clarity, self documenting code and documentation generation.

Naming

Variables

Avoid single character variable names as they won't communicate to a developer what is assigned to the variable and if you combine that with the lack of types it becomes even more of an issue. It should be apparent what a variable contains just by glancing it without having to look at other files, classes or functions.

Errors

Avoid ambiguous names like DuplicateError as they don't provide a whole lot of clarity when going through some code you rarely look at and make it harder to figure out what is trying to be achieved when seeing a bit of code like variable instance DuplicateError.

The first question that would pop to my mind when I see this code would be, what is duplicated? If there would be a check for an error named TransactionAlreadyInPoolError it would be blatantly obvious what is happening and why this error is being looked for.


I'll go over the PR again after the initially requested changes have been made but a great PR that was long overdue now that we have proper IoC thanks for TypeScript and Inversify.

packages/core-transaction-pool/src/query.ts Outdated Show resolved Hide resolved
packages/core-transaction-pool/src/query.ts Outdated Show resolved Hide resolved
packages/core-transaction-pool/src/query.ts Outdated Show resolved Hide resolved
@rainydio

This comment has been minimized.

Copy link
Contributor Author

rainydio commented Feb 14, 2020

Always add types to variables, functions and return values.

Modern languages usually choose inferred types on local variables. Go and Rust were designed this way. C# added var keyword more than 10 years ago. Even conservative Java added var keyword couple of years ago too.

I know that TypeScript and an IDE can just show you all types... If someone is using vim, emacs or some barebones editor like sublime text they will need to dig through the code

This argument of IDE vs plain editor is ages old. I think in this particular case the shift occurred not due to IDEs getting better.

Explicit types hurt readability. They clutter code with information that is only needed if you try to edit it without IDE. For example documentation example snippets (any language) which by definition strike for highest readability use inferred local variable types.

Explicit types hurt code review. Around 5 to 10 years ago online code review tools got popular. The way people read code review diffs is very similar to documentation example snippets. The meaning is in variable and method names, and how they interact. It's difficult to review big chunks of code cluttered with information that's only needed to edit code without IDE.

I'm concerned that explicit local variable types are bad for code. Way more effort is needed to lay down convincing arguments than to add type information.


Regarding one character variable names, you are probably correct. I'm gonna fix that.

Regarding errors, you are probably correct. I used the names that are similar to type field that was already there (e.g. ERR_POOL_FULL translated to PoolFullError). This doesn't have to be that way or type field can be changed too. Next PR will focus on errors and processor and we can discuss this problem there.

@rainydio rainydio force-pushed the feature/segregated-pool branch from 637f13a to 5f41d0a Feb 14, 2020

this.app.bind(Container.Identifiers.TransactionPoolCollator).to(Collator);
this.app.bind(DynamicFeeMatcher).toSelf();
this.app.bind(ExpirationService).toSelf();

This comment has been minimized.

Copy link
@faustbrian

faustbrian Feb 15, 2020

Collaborator

We should avoid self-binding classes and always bind to a symbol/name because this makes it difficult for plugin developers to replace this binding and also makes it harder to swap it out during tests.

.when(Container.Selectors.anyAncestorOrTargetTaggedFirst("state", "pool"));

this.app.bind(Container.Identifiers.TransactionPoolCollator).to(Collator);
this.app.bind(DynamicFeeMatcher).toSelf();

This comment has been minimized.

Copy link
@faustbrian

faustbrian Feb 15, 2020

Collaborator

We should avoid self-binding classes and always bind to a symbol/name because this makes it difficult for plugin developers to replace this binding.

}

this.logger.info(`Received ${Utils.pluralize("transaction", this.transactions.length, true)} (${stats}).`);
await this.networkMonitor.broadcastTransactions(broadcastable);

This comment has been minimized.

Copy link
@faustbrian

faustbrian Feb 15, 2020

Collaborator

This should be moved out so that the sole responsibility of the processor is to decide the state of a transaction (accept, broadcast, excess, invalid, error). The broadcasting is a separate task, the processor should only provide the data that is needed for it.

This comment has been minimized.

Copy link
@rainydio

rainydio Feb 16, 2020

Author Contributor

I disagree. Seems like simple and straightforward solution. Maybe move processor out of core-transaction-pool (but where?). The job of processor is to add to pool, broadcast, and report the results. Instead of creating instructions for caller, let processor do all the job.

This comment has been minimized.

Copy link
@faustbrian

faustbrian Feb 19, 2020

Collaborator

The reasoning is both responsibility and testability. The whole function right now is coupled to the network monitor from the P2P package just for broadcasting which means during tests you have to mock the implementation or fake a response.

99% of this function is attached to the pool, about deciding if transactions are valid and adding them to the pool and then there is this 1 unrelated action in the shape of the P2P call. The transaction pool itself should be unaware of the existence of the P2P layer, it only cares and knows about what transactions are and if they are valid. What the caller wants to do with the transactions after it has handled them is not of any concern for it.

This comment has been minimized.

Copy link
@rainydio

rainydio Feb 19, 2020

Author Contributor

This is actually the reason I think processor should not be part of pool. But more general service on top of pool and p2p (or other broadcast service).

If instead instructions are returned back (previous processor), then someone still has to execute them. There are currently two places that do that, and they did that differently before #3514. P2P was incorrectly(?) refusing the whole batch if single transaction failed to apply, but API broadcasted whatever applied.

It's very easy to make mistake and accidently do the wrong thing. Processor that was implemented in #3514 isn't simple and probably has errors, but everything is contained within it.

This comment has been minimized.

Copy link
@faustbrian

faustbrian Feb 19, 2020

Collaborator

The rejection of everything via P2P even if just 1 transaction is invalid is on purpose as a precaution to prevent someone from sending junk directly to a node via P2P and it causing problems. P2P in general has some different behaviours than the API because the API only cares about taking any data and broadcasting while P2P double checks that everything is 100% valid.

This comment has been minimized.

Copy link
@rainydio

rainydio Feb 19, 2020

Author Contributor

The rejection of everything via P2P even if just 1 transaction is invalid is on purpose as a precaution to prevent someone from sending junk directly to a node via P2P and it causing problems.

It doesn't protect from deliberate attack. Because it's still possible to send same junk through API. Besides processing junk should not cause any problems. The only requirement being that junk isn't broadcasted forward.

P2P in general has some different behaviours than the API because the API only cares about taking any data and broadcasting while P2P double checks that everything is 100% valid.

Why not broadcast 3 out 5 transactions that were valid?


The whole function right now is coupled to the network monitor from the P2P package just for broadcasting which means during tests you have to mock the implementation or fake a response.

You could just mock that broadcast method. But to decouple it, I can create general broadcast service interface and identifier that will be implemented and provided by P2P. So that processor isn't really aware if P2P is performing broadcast (hypothetically it can be list of peer API endpoints).

@faustbrian

This comment has been minimized.

Copy link
Collaborator

faustbrian commented Feb 15, 2020

Modern languages usually choose inferred types on local variables. Go and Rust were designed this way. C# added var keyword more than 10 years ago. Even conservative Java added var keyword couple of years ago too.

This argument of IDE vs plain editor is ages old. I think in this particular case the shift occurred not due to IDEs getting better.

The argument is ages old and still sticks around for a reason. When you jump around dozens of companies you'll meet as many people that don't use IDEs as do use IDEs because they prefer a more minimal setup without all the clutter and noise that modern IDEs add to your development environment.

Explicit types hurt readability. They clutter code with information that is only needed if you try to edit it without IDE. For example documentation example snippets (any language) which by definition strike for highest readability use inferred local variable types.

Explicit types hurt code review. Around 5 to 10 years ago online code review tools got popular. The way people read code review diffs is very similar to documentation example snippets. The meaning is in variable and method names, and how they interact. It's difficult to review big chunks of code cluttered with information that's only needed to edit code without IDE.

I'm concerned that explicit local variable types are bad for code. Way more effort is needed to lay down convincing arguments than to add type information.

Proper naming is obviously key but I would argue the exact opposite in terms of readability and code review problems. If I am required to use an IDE with plugins or dig through files unrelated directly to the changes of a PR to make out what type a variable has or what is returned by a function it is more distracting to me than having the code itself tell me about it.

Could argue that is personal preference but I would say that in general it makes it easier for new people to jump into the code as everything is right at your hand without having to dig deeper to see what class X from 5 dependencies down is returning because the variable doesn't tell me.

Regarding errors, you are probably correct. I used the names that are similar to type field that was already there (e.g. ERR_POOL_FULL translated to PoolFullError). This doesn't have to be that way or type field can be changed too. Next PR will focus on errors and processor and we can discuss this problem there.

If you will create another PR that reworks them right after this is merged it might be better to do them completely in a separate a PR instead of merging something that gets touched up again right after being merged.

@rainydio

This comment has been minimized.

Copy link
Contributor Author

rainydio commented Feb 16, 2020

If you will create another PR that reworks them right after this is merged it might be better to do them completely in a separate a PR instead of merging something that gets touched up again right after being merged.

Merged before (similarly to #3500 and #3501). It's a bit difficult to cut only errors and processor, but let me try.

@faustbrian

This comment has been minimized.

Copy link
Collaborator

faustbrian commented Feb 19, 2020

@rainydio conflicts

}
}

export const describeTransactionType = (transaction: Interfaces.ITransaction): string => {

This comment has been minimized.

Copy link
@faustbrian

faustbrian Feb 19, 2020

Collaborator

Could we make this function more generic so it works for transactions that are developed by third-parties? Right now it would simply return unknown if the transaction isn't an official one that we maintain.

This comment has been minimized.

Copy link
@rainydio

rainydio Feb 19, 2020

Author Contributor

Yes, by adding describe method to transaction handler. But in a separate PR :)

This comment has been minimized.

Copy link
@faustbrian

faustbrian Feb 19, 2020

Collaborator

Please make that PR first so we don't need to merge something that gets removed in a follow up PR.

@rainydio rainydio mentioned this pull request Feb 20, 2020
2 of 3 tasks complete
@faustbrian

This comment has been minimized.

Copy link
Collaborator

faustbrian commented Feb 20, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.