Skip to content

Commit

Permalink
GH-664: HOT FIX ... updating at overflow SQL wrongly written (#224)
Browse files Browse the repository at this point in the history 
* GH-664: fixed the place with the critical issue and also created even better test coverage for receivable_dao.rs; gonna continue at payable_dao.rs too

* GH-664: payable_dao.rs completed and another issue with optional parameters in the overflow sql

* GH-644: neatened, when I had more time to fiddle with the code

* GH-644: Clippy in Node/node

* GH-664: libc changed back to version unspecified + making two tests stronger and sensitive to unintentionally changed rows

Co-authored-by: Bert <[email protected]>
  • Loading branch information
@bertllll
bertllll and Bert authored Jan 23, 2023
1 parent 773fcf0 commit df80ad7
Show file tree
Hide file tree
Showing 5 changed files with 352 additions and 117 deletions.
2 changes: 1 addition & 1 deletion multinode_integration_tests/docker/Dockerfile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
FROM debian:bookworm-slim

RUN apt-get update && \
apt-get install -y libc6=2.36-6 && \
apt-get install -y libc6 && \
# These lines are commented out because for some reason the installation of iptables-persistent hangs forever on
# bullseye-slim. Its absence means that the NodeStartupConfigBuilder::open_firewall_port() function won't work, but
# at the time of this comment it's used in only one place in a way that doesn't have any value. So we decided to
Expand Down
144 changes: 114 additions & 30 deletions node/src/accountant/big_int_processing/big_int_db_processor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ impl<'a, T: TableNameDAO> BigIntDbProcessor<T> {
let mut stm = Self::prepare_statement(conn, main_sql);
let params = config
.params
.merge_pure_rusqlite_and_wei_params((&config.params.wei_change_params).into());
.merge_other_and_wei_params((&config.params.wei_change_params).into());
match stm.execute(params.as_slice()) {
Ok(_) => Ok(()),
//SQLITE_CONSTRAINT_DATATYPE (3091),
Expand Down Expand Up @@ -119,7 +119,7 @@ impl<T: TableNameDAO> UpdateOverflowHandler<T> for UpdateOverflowHandlerReal<T>

let execute_params = config
.params
.merge_pure_rusqlite_and_wei_params(wei_update_array);
.merge_other_and_wei_params_with_conditional_participants(wei_update_array);

Self::execute_update(conn, &config, &execute_params);
Ok(())
Expand Down Expand Up @@ -219,7 +219,7 @@ impl<'a, T: TableNameDAO> BigIntSqlConfig<'a, T> {
}

fn key_param_value(&self) -> &'a dyn ExtendedParamsMarker {
self.params.params_except_wei_change[0].1
self.params.params_except_wei_change[0].value_pair.1
}

fn balance_change(&self) -> i128 {
Expand Down Expand Up @@ -267,7 +267,7 @@ impl_of_extended_params_marker!(i64, &str, Wallet);
pub struct SQLParamsBuilder<'a> {
key_spec_opt: Option<UniqueKeySpec<'a>>,
wei_change_spec_opt: Option<WeiChange>,
other_params: Vec<(&'a str, &'a dyn ExtendedParamsMarker)>,
other_params: Vec<Param<'a>>,
}

impl<'a> SQLParamsBuilder<'a> {
Expand All @@ -286,7 +286,7 @@ impl<'a> SQLParamsBuilder<'a> {
self
}

pub fn other(mut self, params: Vec<(&'a str, &'a (dyn ExtendedParamsMarker + 'a))>) -> Self {
pub fn other(mut self, params: Vec<Param<'a>>) -> Self {
self.other_params = params;
self
}
Expand All @@ -301,7 +301,7 @@ impl<'a> SQLParamsBuilder<'a> {
let ((high_bytes_param_name, low_bytes_param_name), (high_bytes_value, low_bytes_value)) =
Self::expand_wei_params(wei_change_spec);
let key_as_the_first_param = (key_spec.substitution_name_in_sql, key_spec.value_itself);
let params = once(key_as_the_first_param)
let params = once(Param::new(key_as_the_first_param, true))
.chain(self.other_params.into_iter())
.collect();
SQLParams {
Expand Down Expand Up @@ -386,7 +386,7 @@ impl Display for ByteMagnitude {
pub struct SQLParams<'a> {
table_unique_key_name: &'a str,
wei_change_params: WeiChangeAsHighAndLowBytes,
params_except_wei_change: Vec<(&'a str, &'a dyn ExtendedParamsMarker)>,
params_except_wei_change: Vec<Param<'a>>,
}

#[derive(Debug, PartialEq)]
Expand All @@ -407,6 +407,27 @@ impl StdNumParamFormNamed {
}
}

pub struct Param<'a> {
value_pair: (&'a str, &'a dyn ExtendedParamsMarker),
participates_in_overflow_clause: bool,
}

impl<'a> Param<'a> {
pub fn new(
value_pair: (&'a str, &'a (dyn ExtendedParamsMarker + 'a)),
participates_in_overflow_clause: bool,
) -> Self {
Self {
value_pair,
participates_in_overflow_clause,
}
}

fn as_rusqlite_params(&'a self) -> (&'a str, &'a dyn ToSql) {
(self.value_pair.0, &self.value_pair.1 as &dyn ToSql)
}
}

impl<'a> From<&'a WeiChangeAsHighAndLowBytes> for [(&'a str, &'a dyn ToSql); 2] {
fn from(wei_change: &'a WeiChangeAsHighAndLowBytes) -> Self {
[
Expand All @@ -417,22 +438,32 @@ impl<'a> From<&'a WeiChangeAsHighAndLowBytes> for [(&'a str, &'a dyn ToSql); 2]
}

impl<'a> SQLParams<'a> {
fn merge_pure_rusqlite_and_wei_params(
fn merge_other_and_wei_params(
&'a self,
wei_change_params: [(&'a str, &'a dyn ToSql); 2],
) -> Vec<(&'a str, &'a dyn ToSql)> {
self.pure_rusqlite_params(wei_change_params.into_iter())
.collect()
Self::merge_params(self.params_except_wei_change.iter(), wei_change_params)
}

fn pure_rusqlite_params(
fn merge_other_and_wei_params_with_conditional_participants(
&'a self,
wei_change_params: impl Iterator<Item = (&'a str, &'a dyn ToSql)>,
) -> impl Iterator<Item = (&'a str, &'a dyn ToSql)> {
self.params_except_wei_change
wei_change_params: [(&'a str, &'a dyn ToSql); 2],
) -> Vec<(&'a str, &'a dyn ToSql)> {
let preselection = self
.params_except_wei_change
.iter()
.map(|(name, value)| (*name, value as &dyn ToSql))
.chain(wei_change_params)
.filter(|param| param.participates_in_overflow_clause);
Self::merge_params(preselection, wei_change_params)
}

fn merge_params(
params: impl Iterator<Item = &'a Param<'a>>,
wei_change_params: [(&'a str, &'a dyn ToSql); 2],
) -> Vec<(&'a str, &'a dyn ToSql)> {
params
.map(|param| param.as_rusqlite_params())
.chain(wei_change_params.into_iter())
.collect()
}
}

Expand Down Expand Up @@ -546,14 +577,17 @@ mod tests {
sub_name: ":some_key",
val: &"blah",
})
.other(vec![("other_thing", &46565)]);
.other(vec![Param::new(("other_thing", &46565), true)]);

assert_eq!(result.wei_change_spec_opt, Some(Addition("balance", 4546)));
let key_spec = result.key_spec_opt.unwrap();
assert_eq!(key_spec.definition_name, "some_key");
assert_eq!(key_spec.substitution_name_in_sql, ":some_key");
assert_eq!(key_spec.value_itself.to_string(), "blah".to_string());
assert!(matches!(result.other_params[0], ("other_thing", _)));
assert!(matches!(
result.other_params[0].value_pair,
("other_thing", _)
));
assert_eq!(result.other_params.len(), 1)
}

Expand All @@ -568,7 +602,7 @@ mod tests {
sub_name: ":some_key",
val: &"blah",
})
.other(vec![(":other_thing", &11111)])
.other(vec![Param::new((":other_thing", &11111), true)])
.build();

assert_eq!(result.table_unique_key_name, "some_key");
Expand All @@ -579,14 +613,17 @@ mod tests {
low: StdNumParamFormNamed::new(":balance_low_b".to_string(), 115898)
}
);
assert_eq!(result.params_except_wei_change[0].0, ":some_key");
assert_eq!(result.params_except_wei_change[0].value_pair.0, ":some_key");
assert_eq!(
result.params_except_wei_change[0].1.to_string(),
result.params_except_wei_change[0].value_pair.1.to_string(),
"blah".to_string()
);
assert_eq!(result.params_except_wei_change[1].0, ":other_thing");
assert_eq!(
result.params_except_wei_change[1].1.to_string(),
result.params_except_wei_change[1].value_pair.0,
":other_thing"
);
assert_eq!(
result.params_except_wei_change[1].value_pair.1.to_string(),
"11111".to_string()
);
assert_eq!(result.params_except_wei_change.len(), 2)
Expand All @@ -603,7 +640,7 @@ mod tests {
sub_name: ":some_key",
val: &"wooow",
})
.other(vec![(":other_thing", &46565)])
.other(vec![Param::new((":other_thing", &46565), true)])
.build();

assert_eq!(result.table_unique_key_name, "some_key");
Expand All @@ -614,14 +651,17 @@ mod tests {
low: StdNumParamFormNamed::new(":balance_low_b".to_string(), 9223372036854321124)
}
);
assert_eq!(result.params_except_wei_change[0].0, ":some_key");
assert_eq!(result.params_except_wei_change[0].value_pair.0, ":some_key");
assert_eq!(
result.params_except_wei_change[0].1.to_string(),
result.params_except_wei_change[0].value_pair.1.to_string(),
"wooow".to_string()
);
assert_eq!(result.params_except_wei_change[1].0, ":other_thing");
assert_eq!(
result.params_except_wei_change[1].1.to_string(),
result.params_except_wei_change[1].value_pair.0,
":other_thing"
);
assert_eq!(
result.params_except_wei_change[1].value_pair.1.to_string(),
"46565".to_string()
);
assert_eq!(result.params_except_wei_change.len(), 2)
Expand All @@ -634,7 +674,7 @@ mod tests {

let _ = subject
.wei_change(Addition("balance", 4546))
.other(vec![("laughter", &"hahaha")])
.other(vec![Param::new(("laughter", &"hahaha"), true)])
.build();
}

Expand All @@ -649,7 +689,7 @@ mod tests {
sub_name: ":wallet",
val: &make_wallet("wallet"),
})
.other(vec![("other_thing", &46565)])
.other(vec![Param::new(("other_thing", &46565), true)])
.build();
}

Expand All @@ -667,6 +707,50 @@ mod tests {
.build();
}

#[test]
fn merge_other_and_wei_params_with_conditional_participants_can_filter_out_just_update_params()
{
let tuple_matrix = [
("blah", &456_i64 as &dyn ExtendedParamsMarker),
("super key", &"abcxy"),
("time", &779988),
("error", &"no threat"),
];
let update_positive_1 = tuple_matrix[0];
let update_positive_2 = tuple_matrix[1];
let update_negative_1 = tuple_matrix[2];
let update_negative_2 = tuple_matrix[3];
let subject = SQLParams {
table_unique_key_name: "",
wei_change_params: WeiChangeAsHighAndLowBytes {
high: StdNumParamFormNamed {
name: "".to_string(),
value: 0,
},
low: StdNumParamFormNamed {
name: "".to_string(),
value: 0,
},
},
params_except_wei_change: vec![
Param::new(update_positive_2, true),
Param::new(update_negative_1, false),
Param::new(update_positive_1, true),
Param::new(update_negative_2, false),
],
};

let result = subject.merge_other_and_wei_params_with_conditional_participants([
("always_present_1", &12),
("always_present_2", &77),
]);

assert_eq!(result[0].0, update_positive_2.0);
assert_eq!(result[1].0, update_positive_1.0);
assert_eq!(result[2].0, "always_present_1");
assert_eq!(result[3].0, "always_present_2")
}

#[test]
fn return_first_error_works_for_first_error() {
let results = [Err(Error::GetAuxWrongType), Ok(45465)];
Expand Down
7 changes: 7 additions & 0 deletions node/src/accountant/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,13 @@ pub struct ResponseSkeleton {

#[derive(Debug, Message, PartialEq, Eq)]
pub struct ReceivedPayments {
//TODO When we decide whether to delinquency-ban a debtor, we do so based on the age
// of his debt. That age is calculated from the last time he made a payment. It would
// be most accurate to draw that timestamp from the time the block containing the
// payment was placed on the blockchain; however, we're actually drawing the timestamp
// from the moment we discovered and accepted the payment, which is less accurate and
// detects any upcoming delinquency later than the more accurate version would. Is this
// a problem? Do we want to correct the timestamp? Discuss.
pub timestamp: SystemTime,
pub payments: Vec<BlockchainTransaction>,
pub response_skeleton_opt: Option<ResponseSkeleton>,
Expand Down
Loading

0 comments on commit df80ad7

Please sign in to comment.