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

Roy/mempool #43

Merged
merged 153 commits into from
Aug 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
153 commits
Select commit Hold shift + click to select a range
3589f7d
Implement Sum trait for Amount
royster57 Dec 9, 2021
7d01158
Make Transaction errors thiserror
royster57 Jan 30, 2022
e315416
Conversion from Amount to underlying u128 type
royster57 Jan 30, 2022
b612cdb
Basic Mempool Functionality
royster57 Feb 22, 2022
35930a2
Move get outpoint value (Test pass)
royster57 Feb 11, 2022
40cf3a0
Order TxMempoolEntry by Hash, not Fee
royster57 Feb 14, 2022
4444a86
Fix DFS in unconfirmed ancestors_search
royster57 Feb 14, 2022
82e3adf
Allow multiple txs with the same fee
royster57 Feb 14, 2022
736cd4b
Refactor fee calculation
royster57 Feb 14, 2022
76e2d11
Support txs with prescribed fees in tests
royster57 Feb 14, 2022
5f6b788
Refactor TxMempoolEntry to make it unit-testable
royster57 Feb 15, 2022
4f17792
Add unit test for TxMempoolEntry
royster57 Feb 15, 2022
da02748
Added unit test of TxMempoolEntry DFS
royster57 Feb 15, 2022
eb59292
Refactor test utilityfunction tx_spend_input
royster57 Feb 15, 2022
b93eb87
replacement fee higher than original in unit test
royster57 Feb 15, 2022
7514284
Refactor call to is_replaceable in validation
royster57 Feb 15, 2022
b77fb9a
Add children to TxMempoolEntry
royster57 Feb 15, 2022
94667e7
Need to figure out how to update parents
royster57 Feb 15, 2022
476d2e4
Check that replacement tx pays more than conflict
royster57 Feb 15, 2022
a7c619c
Add TODO to test tx_replace_child
royster57 Feb 16, 2022
4b26371
Use H256 instead of Rc to reference entries in the mempool
royster57 Feb 17, 2022
150c9bd
Getters for parents and children
royster57 Feb 17, 2022
5ffaa81
Add convenience method TxMempoolEntry::tx_id
royster57 Feb 17, 2022
787200e
Refactor test tx_replace_child
royster57 Feb 17, 2022
de77505
Expand tx_replace test
royster57 Feb 17, 2022
2b502e9
Move TxMepoolEntry next to impl
royster57 Feb 17, 2022
ba417d6
Support count_with_descendants in TxMempoolEntry
royster57 Feb 18, 2022
c5bab45
Validate number of potential replacements
royster57 Feb 18, 2022
5ca71c9
Rename 'get_parents' to 'unconfirmed_parents'
royster57 Feb 18, 2022
f58473f
Refactor parent update
royster57 Feb 18, 2022
8f84402
Refactor ancestor count update
royster57 Feb 18, 2022
98e8912
Refactor marking outpoints as spent
royster57 Feb 18, 2022
dcbc97b
Remove local variable in add_tx
royster57 Feb 18, 2022
f2d69b0
Make ReplacementFeeLowerThanOriginal error more descriptive
royster57 Feb 18, 2022
9f7499d
Fixed tx_spend_inputs
royster57 Feb 18, 2022
9495d19
Fix tx_replace_child test
royster57 Feb 18, 2022
29db54c
Refactor, expand, and rename tx_mempool_entry test
royster57 Feb 18, 2022
d1253b9
fee member of TxGenerator does not need to be an option
royster57 Feb 18, 2022
0eb6ec9
Move MAX_BIP125_REPLACEMENT_CANDIDATES to module scope, for testing
royster57 Feb 18, 2022
232f9b0
BIP125 max replacements test
royster57 Feb 18, 2022
d6cf6c6
Refactor rbf checks into separate function
royster57 Feb 18, 2022
ee5ab37
Refactor tx_spend_several_inputs
royster57 Feb 21, 2022
ccf59f3
ChainState trait inherits Debug
royster57 Feb 21, 2022
d4af46a
Validation - Replacing Tx spends no new unconfirmed outputs
royster57 Feb 21, 2022
ea0de8e
Clarity: Renaming and Comments
royster57 Feb 22, 2022
d6f518a
Validation: Replacement Tx pays more than conflicts together with their
royster57 Feb 22, 2022
99e3564
Test pays_more_than_conflicts_with_descendants
royster57 Feb 22, 2022
bcb0577
derive(Debug) for test struct ValuedOutPoint
royster57 Feb 23, 2022
dce5337
Add comment and reorder RBF validation calls
royster57 Feb 23, 2022
0bb2585
pays_more_than_conflicts_with_descendant returns total fee of conflic…
royster57 Feb 23, 2022
3c4641b
Validation: replacement tx pays for its bandwidth
royster57 Feb 23, 2022
78fd4f5
Accomodate for relay fee checks in older tests
royster57 Feb 23, 2022
5c20791
Refactor TxGenerator: builder pattern for num_inputs, num_outputs
royster57 Feb 23, 2022
194f4eb
TxGenerator: builder pattern for replaceability flag
royster57 Feb 23, 2022
cfe46ac
Remove outdated comments
royster57 Feb 23, 2022
342f073
wip delete TxGenerator::new and rename TxGenerator::new_with_unconfir…
royster57 Feb 23, 2022
d09cf9d
Make test_replace_tx use tx_spend_input instead of TxGenerator
royster57 Feb 23, 2022
592801c
Rename MempoolError to simply Error within the context of the crate
royster57 Feb 24, 2022
80d370a
Bug Fix: move rbf checks out of conflict loop
royster57 Feb 24, 2022
93dbbe1
Refactor rbf_checks
royster57 Feb 24, 2022
b6f19b9
Mempool: Add comment
royster57 Feb 24, 2022
423be74
Add function get_relay_fee
royster57 Feb 24, 2022
2b5bcf6
Validation: minimum_relay_fees
royster57 Feb 24, 2022
a6008e4
TESTS: accomodate for minimum relay fee check
royster57 Feb 24, 2022
7280e16
TESTS: Fix txs_sorted (compute fees properly)
royster57 Feb 25, 2022
6d356da
TxMempoolEntry: new types for Ancestors and Descendants
royster57 Feb 25, 2022
9984a49
More precise errors for tx fee computation
royster57 Mar 1, 2022
94f3795
Add pow function to amount
royster57 Mar 7, 2022
9fecf33
Add finalize_tx and move entry creation into it
royster57 Mar 7, 2022
dee73dc
Return conflicts from validation
royster57 Mar 7, 2022
9c8de4a
Drop conflicts when adding validated tx to mempool
royster57 Mar 7, 2022
c999b3c
TESTS: verify conflicts are removed
royster57 Mar 8, 2022
70a9da9
Fix bug in confirmed_outpoints
royster57 Mar 15, 2022
bd2145f
replace expect with error when when fee higher than inputs
royster57 Mar 15, 2022
49404a4
Replace expect calls with errors
royster57 Mar 16, 2022
86c6573
Increase min, max output values in unit tests to make more deterministic
royster57 Mar 15, 2022
17eb185
Rename ChainStateMock's members
royster57 Mar 15, 2022
190fcdb
Implement Rolling Fee Rate and eviction of expired entries
royster57 Mar 7, 2022
51abf64
rename random_unspent_outpoint -> get_unspent_outpoint
royster57 Mar 17, 2022
ef14b0e
Make InsufficientFeesToRelay error more informative
royster57 Mar 17, 2022
a716afe
TESTS: TxGenerator fee handling
royster57 Mar 17, 2022
c660762
TESTS: Add manual implementation of Ord for ValuedOutPoint so coin_po…
royster57 Mar 17, 2022
1c1cdab
TESTS: infrastructure improvements
royster57 Mar 17, 2022
7c85942
real_size_tx test
royster57 Mar 17, 2022
772a4e7
Add test for different_size_txs
royster57 Mar 17, 2022
970661b
Replace println calls with logging
royster57 Mar 18, 2022
fbc3723
Delete old TODOs and constant pertaining to mempool fullness
royster57 Mar 18, 2022
63052f9
Use logging in mempool tests
royster57 Mar 21, 2022
6ce5bdd
Improve test function generate_tx_outputs
royster57 Mar 31, 2022
6425ada
make unspend_outpoints a function
royster57 Apr 4, 2022
577ce24
derive Debug for Ancestors, Descendants, Conflicts
royster57 Apr 1, 2022
f016eb4
Add fees_with_descendant field to TxMempoolEntry
royster57 Mar 30, 2022
438eee8
Rename txs_by_fee -> txs_by_descendant_score
royster57 Mar 31, 2022
870a636
new function update_ancestor_state
royster57 Mar 30, 2022
136f300
Update fees_with_descendants field of ancestors when new entry is
royster57 Mar 30, 2022
6f2d69a
remove outdated TODO
royster57 Mar 31, 2022
2d77727
Introduce DescendantScore type and sort according to it
royster57 Apr 1, 2022
80dd611
Update descendant score index when an entry is dropped
royster57 Apr 4, 2022
487ec7e
Refactor and rename udpate_ancestor_state -> update_ancestor_state_fo…
royster57 Apr 4, 2022
a30b7bd
Update ancestor state when entry is dropped
royster57 Apr 4, 2022
98125c2
TESTS: descendant score
royster57 Mar 31, 2022
aa3e2af
TESTS: add test try_replace_irreplaceable
royster57 Apr 4, 2022
7d1434c
Refactor finalize_tx
royster57 Apr 5, 2022
e8cb3c4
Proper error for descendant of expired transaction
royster57 Apr 5, 2022
e143936
TESTS: descendant of expired entry
royster57 Apr 5, 2022
027d82c
TESTS: mempool full
royster57 Apr 5, 2022
7961096
TESTS: no empty bags in descendant score index
royster57 Apr 5, 2022
6136347
Mempool: Documentation
royster57 Apr 5, 2022
b564d87
Improve style in ancestor and descendant functions
royster57 Apr 11, 2022
2cab6ce
Fix FeeRate calculations to consistently use tokens/kb and update
royster57 Apr 11, 2022
c6f2cd5
Remove Amount::random, which is no longer needed
royster57 Apr 12, 2022
b4f6dc2
TESTS Remove time measurements from different_size_txs
royster57 Apr 12, 2022
61c9daa
Use u128 as FeeRate's underlying type
royster57 Apr 13, 2022
b3d5a66
FeeRate multiply before divide in computing tokens_per_kb
royster57 Apr 18, 2022
fe8020e
Replace unwrap calls with expect
royster57 Apr 18, 2022
7b1c815
Fix tests that broke during rebase
royster57 Jul 21, 2022
c02d1db
Move mempool tests to separate file
royster57 Jul 26, 2022
b054ca5
Create FeeRate from Amount rather than integer type
royster57 Jul 26, 2022
b0ffd64
Rename tokens_per_kb field of FeeRate to atoms_per_kb
royster57 Jul 26, 2022
0f0365d
Style changes to newtype macro
royster57 Jul 26, 2022
3eba529
Use exp2 method in rolling fee rate calculation
royster57 Jul 26, 2022
0dbcac4
Use into_atoms to convert Amount to u128
royster57 Jul 26, 2022
8fb0b6c
Update scale codec to 3.1
royster57 Jul 26, 2022
ac5e23c
Test sum of empty Amount vector
royster57 Jul 26, 2022
7170066
Clean up compute_fee
royster57 Jul 26, 2022
c5fa3a2
Use saturating_sub when computing expired entries
royster57 Jul 26, 2022
07019c9
Add test for FeeRate::div_up
royster57 Jul 27, 2022
cd04ceb
Add comment to time_lock_notes.txt
royster57 Jul 27, 2022
40c45b7
Use NonZeroU128 as FeeRate divisor
royster57 Jul 27, 2022
5eb09bc
Remove unnecessary static bounds
royster57 Jul 28, 2022
98e8e4a
Rename test one_ancestor_signal_is_enough
royster57 Jul 28, 2022
b3b0244
Rename tokens_per_kb -> atoms_per_kb
royster57 Jul 28, 2022
3585773
Rename FeeRate::of_tx -> FeeRate::from_total_tx_fees
royster57 Jul 28, 2022
122479f
Remove eprintln calls
royster57 Jul 28, 2022
f99611e
Use ensure macro
royster57 Jul 28, 2022
205b79e
Fix indentation in newtype example
royster57 Jul 28, 2022
491585d
Friendlier human-readable errors for mempool
royster57 Jul 28, 2022
01f766c
Use Id<Transaction> instead of H256 where possible
royster57 Jul 28, 2022
168509f
Use ensure in mempool
royster57 Aug 1, 2022
56e4da2
Use from attribute to generate conversion TxValidationError->MempoolE…
royster57 Aug 1, 2022
b98edf8
Suppress clippy float arithmetic warning
royster57 Aug 1, 2022
48d61d0
Make PartialEq, Ord implementations consistent for TxMempoolEntry
royster57 Aug 1, 2022
91947ce
Correct order of operations in feerate calculation
royster57 Aug 1, 2022
51fc129
Round up in fee computation
royster57 Aug 1, 2022
7865336
Add documentation
royster57 Aug 1, 2022
b15c868
Fix test tx_too_big
royster57 Aug 1, 2022
c7fea8b
Move test machinery to different submodule
royster57 Aug 1, 2022
895615e
Refactor drop_tx
royster57 Aug 2, 2022
bf2fae0
Improvements to FeeRate::div_up
royster57 Aug 2, 2022
7de1ead
Remove no longer needed assert
royster57 Aug 2, 2022
919df10
FeeRate division used only in tests
royster57 Aug 2, 2022
bfa9325
Mempool TODOs
royster57 Aug 2, 2022
a78b0cf
Update deny.toml
royster57 Aug 3, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
387 changes: 222 additions & 165 deletions Cargo.lock

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ members = [
"chainstate", # code on chainstate of blocks and transactions
"script", # bitcoin script and its interfaces
"logging", # logging engine and its interfaces
"mempool", # mempool interface and implementation
"p2p", # p2p communication interfaces and protocols
"rpc", # rpc abstraction and implementation
"serialization", # full featured serialization interfaces and implementations
Expand All @@ -40,6 +41,7 @@ default-members = [
"chainstate",
"script",
"logging",
"mempool",
"p2p",
"rpc",
"serialization",
Expand All @@ -61,6 +63,7 @@ chainstate = { path = "chainstate"}
chainstate-types = { path = "chainstate-types"}
script = { path = "script"}
logging = { path = "logging"}
mempool = { path = "mempool"}
p2p = { path = "p2p"}
rpc = { path = "rpc"}
serialization = { path = "serialization"}
Expand Down
4 changes: 3 additions & 1 deletion common/src/chain/transaction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

use crate::primitives::{Id, Idable};
use serialization::{DirectDecode, DirectEncode, Encode};
use thiserror::Error;

use crate::chain::transaction::transaction_v1::TransactionV1;

Expand Down Expand Up @@ -54,8 +55,9 @@ impl Idable for Transaction {
}
}

#[derive(Debug, Clone)]
#[derive(Error, Debug, Clone)]
pub enum TransactionCreationError {
#[error("An unknown error has occurred")]
Unknown,
}

Expand Down
9 changes: 9 additions & 0 deletions common/src/chain/transaction/input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,15 @@ impl From<Id<Genesis>> for OutPointSourceId {
}
}

impl OutPointSourceId {
pub fn get_tx_id(&self) -> Option<&Id<Transaction>> {
match self {
OutPointSourceId::Transaction(id) => Some(id),
_ => None,
}
}
}

#[derive(Debug, Clone, PartialEq, Eq, Encode, Decode)]
pub struct OutPoint {
id: OutPointSourceId,
Expand Down
29 changes: 29 additions & 0 deletions common/src/primitives/amount.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,35 @@ mod tests {
assert_eq!(Amount { val: IntType::MAX } + Amount { val: 1 }, None);
}

#[test]
fn sum_some() {
let amounts = vec![Amount { val: 1 }, Amount { val: 2 }, Amount { val: 3 }];
assert_eq!(
amounts.into_iter().sum::<Option<Amount>>(),
Some(Amount { val: 6 })
);
}

#[test]
fn sum_overflow() {
let amounts = vec![
Amount { val: 1 },
Amount { val: 2 },
Amount {
val: IntType::MAX - 2,
},
];
assert_eq!(amounts.into_iter().sum::<Option<Amount>>(), None);
}

#[test]
fn sum_empty() {
assert_eq!(
vec![].into_iter().sum::<Option<Amount>>(),
Some(Amount::from_atoms(0))
)
}

#[test]
fn sub_underflow() {
assert_eq!(Amount { val: IntType::MIN } - Amount { val: 1 }, None);
Expand Down
2 changes: 2 additions & 0 deletions deny.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ skip = [
{name = "textwrap"},
{name = "wasi"},
{name = "winapi"},
{name = "windows"},
{name = "windows_aarch64_msvc" },
{name = "windows_i686_gnu" },
{name = "windows_i686_msvc" },
Expand All @@ -70,6 +71,7 @@ allow = [
"LicenseRef-webpki",
"WTFPL",
"BSL-1.0",
"Unicode-DFS-2016",
"Unlicense",#this is a specific license rather than no license at all
] #deny a license not in this set of licenses

Expand Down
18 changes: 18 additions & 0 deletions mempool/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
[package]
name = "mempool"
version = "0.1.0"
edition = "2021"
license = "MIT"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
parity-scale-codec = {version = "3.1", features = ["derive", "chain-error"]}
serialization = { path = '../serialization' }
common = { path = '../common' }
utils = { path = '../utils' }
logging = { path = '../logging' }
anyhow = "1.0"
thiserror = "1.0"
lazy_static = "1.4"
mockall = "0.11.0"
71 changes: 71 additions & 0 deletions mempool/src/error.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
use thiserror::Error;

use common::chain::transaction::Transaction;
use common::chain::OutPoint;
use common::primitives::amount::Amount;
use common::primitives::Id;
use common::primitives::H256;

#[derive(Debug, Error)]
pub enum Error {
#[error("Mempool is full")]
MempoolFull,
#[error(transparent)]
TxValidationError(#[from] TxValidationError),
}

#[derive(Debug, Error)]
pub enum TxValidationError {
#[error("Transaction has no inputs.")]
NoInputs,
#[error("Transaction has no outputs.")]
NoOutputs,
#[error("Transaction has duplicate inputs.")]
DuplicateInputs,
#[error("Outpoint not found: {outpoint:?}")]
OutPointNotFound {
outpoint: OutPoint,
tx_id: Id<Transaction>,
},
#[error("Transaction exceeds the maximum block size.")]
ExceedsMaxBlockSize,
#[error("Transaction already exists in the mempool.")]
TransactionAlreadyInMempool,
#[error("Transaction conflicts with another, irreplaceable transaction.")]
ConflictWithIrreplaceableTransaction,
#[error("The sum of the transaction's inputs' values overflows.")]
InputValuesOverflow,
#[error("The sum of the transaction's outputs' values overflows.")]
OutputValuesOverflow,
#[error("The sum of the transaction's inputs is smaller than the sum of its outputs.")]
InputsBelowOutputs,
#[error("Replacement transaction has fee lower than the original. Replacement fee is {replacement_fee:?}, original fee {original_fee:?}")]
ReplacementFeeLowerThanOriginal {
replacement_tx: H256,
replacement_fee: Amount,
original_tx: H256,
original_fee: Amount,
},
#[error("Transaction would require too many replacements.")]
TooManyPotentialReplacements,
#[error("Replacement transaction spends an unconfirmed input which was not spent by any of the original transactions.")]
SpendsNewUnconfirmedOutput,
#[error("The sum of the fees of this transaction's conflicts overflows.")]
ConflictsFeeOverflow,
#[error("Transaction pays a fee that is lower than the fee of its conflicts with their descendants.")]
TransactionFeeLowerThanConflictsWithDescendants,
#[error("Underflow in computing transaction's additional fees.")]
AdditionalFeesUnderflow,
#[error("Transaction does not pay sufficient fees to be relayed.")]
InsufficientFeesToRelay { tx_fee: Amount, relay_fee: Amount },
#[error("Replacement transaction does not pay enough for its bandwidth.")]
InsufficientFeesToRelayRBF,
#[error("Rolling fee threshold not met.")]
RollingFeeThresholdNotMet { minimum_fee: Amount, tx_fee: Amount },
#[error("Overflow encountered while updating ancestor fee.")]
AncestorFeeUpdateOverflow,
#[error("Transaction is a descendant of expired transaction.")]
DescendantOfExpiredTransaction,
#[error("Internal Error.")]
InternalError,
}
76 changes: 76 additions & 0 deletions mempool/src/feerate.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
use common::primitives::amount::Amount;

lazy_static::lazy_static! {
pub(crate) static ref INCREMENTAL_RELAY_FEE_RATE: FeeRate = FeeRate::new(Amount::from_atoms(1000));
pub(crate) static ref INCREMENTAL_RELAY_THRESHOLD: FeeRate = FeeRate::new(Amount::from_atoms(500));
}

// TODO we should reconsider using Amount and define functions for the specific operations required
// on FeeRate. Previous attempts to do this did not pan out, but this should be revisited to avoid
// dangerous arithmetic operations
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
pub(crate) struct FeeRate {
atoms_per_kb: u128,
}

impl FeeRate {
pub(crate) fn new(atoms_per_kb: Amount) -> Self {
Self {
atoms_per_kb: atoms_per_kb.into_atoms(),
}
}

pub(crate) fn from_total_tx_fee(total_tx_fee: Amount, tx_size: usize) -> Self {
Self {
atoms_per_kb: Self::div_up(1000 * total_tx_fee.into_atoms(), tx_size),
}
Copy link
Contributor

@iljakuklic iljakuklic Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two issues:

  1. The point of multiplying by 1000 is to represent the feerate with higher precision. Dividing by the number of bytes first and returning the rounded value makes the subsequent multiplication by 1000 rather pointless. Should be something like: (1000 * tokens) / bytes rather than 1000 * (tokens / bytes). It makes a difference because we use integer division.
  2. We have to be careful about overflow. Say someone issues a token with total supply of 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.

Copy link
Contributor

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.

Copy link
Contributor Author

@royster57 royster57 Apr 18, 2022

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?

Copy link
Contributor

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.

Copy link
Contributor

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:

  • Somehow limit the number of atoms (of any token) spent on fees to u128::MAX (or less). Then somehow make sure the rule is enforced before it enters this calculation, which can be done with types.
  • Use even bigger type than 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?

}

pub(crate) fn compute_fee(&self, size: usize) -> Amount {
let size = u128::try_from(size).expect("compute_fee conversion");
// +999 for ceil operation
Amount::from_atoms((self.atoms_per_kb * size + 999) / 1000)
}

pub(crate) fn atoms_per_kb(&self) -> u128 {
self.atoms_per_kb
}

// TODO Use NonZeroUsize for divisor
fn div_up(dividend: u128, divisor: usize) -> u128 {
debug_assert!(divisor != 0);
let divisor = u128::try_from(divisor).expect("div_up conversion");
(dividend + divisor - 1) / divisor
}
}

impl std::ops::Add for FeeRate {
type Output = FeeRate;
fn add(self, other: Self) -> Self::Output {
let atoms_per_kb = self.atoms_per_kb + other.atoms_per_kb;
FeeRate { atoms_per_kb }
}
}

#[cfg(test)]
mod tests {
use super::*;
use std::num::NonZeroU128;

impl std::ops::Div<NonZeroU128> for FeeRate {
type Output = FeeRate;
fn div(self, rhs: NonZeroU128) -> Self::Output {
FeeRate {
atoms_per_kb: self.atoms_per_kb / rhs,
}
}
}

#[test]
fn test_div_up() {
let fee = 7;
let tx_size = usize::MAX;
let rate = FeeRate::div_up(fee, tx_size);
assert_eq!(rate, 1);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also test where fee is u128::MAX would be nice

Copy link
Contributor Author

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?

Copy link
Contributor

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:

  1. Return a Result with Err for values that cannot be handled, or
  2. clearly document in doc comments (by convention, Panics section is used for that) the range of acceptable values and failure modes and justify in call sites why it cannot happen, or
  3. changed so it handles values around u128::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.

}
7 changes: 7 additions & 0 deletions mempool/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#![deny(clippy::clone_on_ref_ptr)]

pub mod error;
mod feerate;
pub mod pool;

pub use error::Error as MempoolError;
Loading