Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Roy/mempool #43
Roy/mempool #43
Changes from all commits
3589f7d
7d01158
e315416
b612cdb
35930a2
40cf3a0
4444a86
82e3adf
736cd4b
76e2d11
5f6b788
4f17792
da02748
eb59292
b93eb87
7514284
b77fb9a
94667e7
476d2e4
a7c619c
4b26371
150c9bd
5ffaa81
787200e
de77505
2b502e9
ba417d6
c5bab45
5ca71c9
f58473f
8f84402
98e8912
dcbc97b
f2d69b0
9f7499d
9495d19
29db54c
d1253b9
0eb6ec9
232f9b0
d6cf6c6
ee5ab37
ccf59f3
d4af46a
ea0de8e
d6f518a
99e3564
bcb0577
dce5337
0bb2585
3c4641b
78fd4f5
5c20791
194f4eb
cfe46ac
342f073
d09cf9d
592801c
80d370a
93dbbe1
b6f19b9
423be74
2b5bcf6
a6008e4
7280e16
6d356da
9984a49
94f3795
9fecf33
dee73dc
9c8de4a
c999b3c
70a9da9
bd2145f
49404a4
86c6573
17eb185
190fcdb
51abf64
ef14b0e
a716afe
c660762
1c1cdab
7c85942
772a4e7
970661b
fbc3723
63052f9
6ce5bdd
6425ada
577ce24
f016eb4
438eee8
870a636
136f300
6f2d69a
2d77727
80dd611
487ec7e
a30b7bd
98125c2
aa3e2af
7d1434c
e8cb3c4
e143936
027d82c
7961096
6136347
b564d87
2cab6ce
c6f2cd5
b4f6dc2
61c9daa
b3d5a66
fe8020e
7b1c815
c02d1db
b054ca5
b0ffd64
0f0365d
3eba529
0dbcac4
8fb0b6c
ac5e23c
7170066
c5fa3a2
07019c9
cd04ceb
40c45b7
5eb09bc
98e8e4a
b3b0244
3585773
122479f
f99611e
205b79e
491585d
01f766c
168509f
56e4da2
b98edf8
48d61d0
91947ce
51fc129
7865336
b15c868
c7fea8b
895615e
bf2fae0
7de1ead
919df10
bfa9325
a78b0cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two issues:
(1000 * tokens) / bytes
rather than1000 * (tokens / bytes)
. It makes a difference because we use integer division.u128::MAX
and the transaction and pays out all of it in fees in one transaction. Such transaction can weigh realistically around 120 bytes. The feerate internal representation (1000 * (u128::MAX / 120)) overflows. We should limit either the total supply of tokens or the maximum fee to u128::MAX / 1000.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing any unchecked math operations without proper Result/Option handling is basically playing with fire. This is exactly why I didn't want to allow releasing integers from Amount... it's like it's inevitable for people to do these mistakes again, and again and again. I don't know how paranoid we have to become... maybe we should just ban using normal integers and write a generic wrapper that only allows checked ops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iljakuklic the first point you made is easy enough to fix but the second is a decision I have no strong feeling about since
u128::MAX
sounds to me such a huge number. @TheQuantumPhysicist What do you think regarding Lukas's second point?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem isn't specifically large or small number. But the problem is that we're developing a premissionless public software. My fear is always that some operations are directly accessible to the public and can be used to crash nodes or produce undefined behavior.
Even worse. The problem is that even if the code is checked now and ensure that it's never gonna overflow because of a prior function, my paranoia still makes me think that a future refactor might reintroduce the problem, because people typically don't look at other code when they refactor, but rather only look at the code they're changing. To solve that problem, I either write tests specifically prove that this will never happen for the function in question, or make the function self-protecting such that I don't have to worry about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any thoughts on how to resolve this issue. It may be fine with MLTs but with the ability to pay fees in any token, issuing a token with
u128::MAX
and spending it on fees will become a viable attack vector.Even if we only allow MLTs for the time of being, we should think whether this is possible to exploit. I cannot see how it is at the moment: Fee is calculated from total input amounts so it cannot be larger than total MLT supply (unless there is a bug somewhere else). I have not, however, run through all possible code paths so I am still quite uncomfortable with the overflow possibility.
Ultimately, the solution is one of these, as far as I can see:
u128::MAX
(or less). Then somehow make sure the rule is enforced before it enters this calculation, which can be done with types.u128
to accommodate for the extra precision without overflow. That would require find something in an external library or define a custom type to do it, with all the necessary arithmetic on it. (u128
,u16
) =~u144
should be good enough 😆Any more observations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also test where
fee
isu128::MAX
would be niceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using
u128::MAX
for fee causes overflow. What exactly should we test here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
div_up
function should either:Result
withErr
for values that cannot be handled, orPanics
section is used for that) the range of acceptable values and failure modes and justify in call sites why it cannot happen, oru128::MAX
correctly too.Since (3) is not too difficult to implement (see my comment on the
div_up
function) and people rarely read docs, it would be my preferred option.