Skip to content

Fix orders disappearing from orderbook #1046

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.

Sign up for GitHub

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

Merged
sergeyboyko0791 merged 5 commits into from
Aug 20, 2021
Merged

Fix orders disappearing from orderbook #1046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4ed62d1
fix pair trie to be same for old and new nodes
shamardy Aug 17, 2021
8c1d68e
Merge remote-tracking branch 'origin/dev' into disappearing-orders-fix
shamardy Aug 18, 2021
dbb8c0a
rename function to clone_without_protocol_info
shamardy Aug 18, 2021
aefe460
old nodes to see segwit orders but can not match
shamardy Aug 18, 2021
740f580
Remove protocol_info from OrderbookItem/MakerOrder
shamardy Aug 19, 2021
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
1 change: 1 addition & 0 deletions mm2src/docker_tests/qrc20_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1846,6 +1846,7 @@ pub async fn enable_native_segwit(mm: &MarketMakerIt, coin: &str) -> Json {
}

#[test]
#[ignore]
fn segwit_address_in_the_orderbook() {
wait_for_estimate_smart_fee(30).expect("!wait_for_estimate_smart_fee");
// generate QTUM coin with the dynamic fee and fill the wallet by 0.5 Qtums
Expand Down
97 changes: 11 additions & 86 deletions mm2src/lp_ordermatch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,12 +101,11 @@ impl From<(new_protocol::MakerOrderCreated, String)> for OrderbookItem {
min_volume: order.min_volume,
uuid: order.uuid.into(),
created_at: order.created_at,
base_protocol_info: order.base_protocol_info,
rel_protocol_info: order.rel_protocol_info,
}
}
}

#[allow(dead_code)]
pub fn addr_format_from_protocol_info(protocol_info: &Option<Vec<u8>>) -> AddressFormat {
match protocol_info {
Some(info) => match rmp_serde::from_read_ref::<_, AddressFormat>(info) {
Expand Down Expand Up @@ -251,7 +250,6 @@ async fn request_and_fill_orderbook(ctx: &MmArc, base: &str, rel: &str) -> Resul
let request = OrdermatchRequest::GetOrderbook {
base: base.to_string(),
rel: rel.to_string(),
version: OrdermatchRequestVersion::V2,
};

let response = try_s!(request_any_relay::<GetOrderbookRes>(ctx.clone(), P2PRequest::Ordermatch(request)).await);
Expand Down Expand Up @@ -409,24 +407,12 @@ pub async fn process_msg(ctx: MmArc, _topics: Vec<String>, from_peer: String, ms
}
}

#[derive(Debug, Deserialize, Eq, PartialEq, Serialize)]
pub enum OrdermatchRequestVersion {
V1,
V2,
}

impl Default for OrdermatchRequestVersion {
fn default() -> Self { OrdermatchRequestVersion::V1 }
}

#[derive(Debug, Deserialize, Eq, PartialEq, Serialize)]
pub enum OrdermatchRequest {
/// Get an orderbook for the given pair.
GetOrderbook {
base: String,
rel: String,
#[serde(default)]
version: OrdermatchRequestVersion,
},
/// Sync specific pubkey orderbook state if our known Patricia trie state doesn't match the latest keep alive message
SyncPubkeyOrderbookState {
Expand All @@ -438,8 +424,6 @@ pub enum OrdermatchRequest {
coin: String,
action: BestOrdersAction,
volume: BigRational,
#[serde(default)]
version: OrdermatchRequestVersion,
},
OrderbookDepth {
pairs: Vec<(String, String)>,
Expand Down Expand Up @@ -486,19 +470,14 @@ impl TryFromBytes for Uuid {
pub async fn process_peer_request(ctx: MmArc, request: OrdermatchRequest) -> Result<Option<Vec<u8>>, String> {
log::debug!("Got ordermatch request {:?}", request);
match request {
OrdermatchRequest::GetOrderbook { base, rel, version } => {
process_get_orderbook_request(ctx, base, rel, version).await
},
OrdermatchRequest::GetOrderbook { base, rel } => process_get_orderbook_request(ctx, base, rel).await,
OrdermatchRequest::SyncPubkeyOrderbookState { pubkey, trie_roots } => {
let response = process_sync_pubkey_orderbook_state(ctx, pubkey, trie_roots).await;
response.map(|res| res.map(|r| encode_message(&r).expect("Serialization failed")))
},
OrdermatchRequest::BestOrders {
coin,
action,
volume,
version,
} => best_orders::process_best_orders_p2p_request(ctx, coin, action, volume, version).await,
OrdermatchRequest::BestOrders { coin, action, volume } => {
best_orders::process_best_orders_p2p_request(ctx, coin, action, volume).await
},
OrdermatchRequest::OrderbookDepth { pairs } => {
orderbook_depth::process_orderbook_depth_p2p_request(ctx, pairs).await
},
Expand All @@ -523,12 +502,7 @@ struct GetOrderbookRes {
pubkey_orders: HashMap<String, GetOrderbookPubkeyItem>,
}

async fn process_get_orderbook_request(
ctx: MmArc,
base: String,
rel: String,
version: OrdermatchRequestVersion,
) -> Result<Option<Vec<u8>>, String> {
async fn process_get_orderbook_request(ctx: MmArc, base: String, rel: String) -> Result<Option<Vec<u8>>, String> {
fn get_pubkeys_orders(orderbook: &Orderbook, base: String, rel: String) -> (usize, HashMap<String, PubkeyOrders>) {
let asks = orderbook.unordered.get(&(base.clone(), rel.clone()));
let bids = orderbook.unordered.get(&(rel, base));
Expand Down Expand Up @@ -569,23 +543,9 @@ async fn process_get_orderbook_request(
pubkey
))?;

let filtered_orders: Vec<(Uuid, OrderbookItem)> = orders
.into_iter()
.filter_map(|(uuid, order)| {
if version == OrdermatchRequestVersion::V1
&& (order.base_protocol_info.is_some() || order.base_protocol_info.is_some())
{
log::debug!("Order {} address format is not supported by receiver", order.uuid);
None
} else {
Some((uuid, order))
}
})
.collect::<Vec<_>>();

let item = GetOrderbookPubkeyItem {
last_keep_alive: pubkey_state.last_keep_alive,
orders: filtered_orders,
orders,
// TODO save last signed payload to pubkey state
last_signed_pubkey_payload: vec![],
};
Expand Down Expand Up @@ -773,12 +733,7 @@ fn test_parse_orderbook_pair_from_topic() {
assert_eq!(None, parse_orderbook_pair_from_topic("orbk/BTC:"));
}

async fn maker_order_created_p2p_notify(
ctx: MmArc,
order: &MakerOrder,
base_protocol_info: Option<Vec<u8>>,
rel_protocol_info: Option<Vec<u8>>,
) {
async fn maker_order_created_p2p_notify(ctx: MmArc, order: &MakerOrder) {
let topic = orderbook_topic_from_base_rel(&order.base, &order.rel);
let message = new_protocol::MakerOrderCreated {
uuid: order.uuid.into(),
Expand All @@ -791,8 +746,6 @@ async fn maker_order_created_p2p_notify(
created_at: now_ms() / 1000,
timestamp: now_ms() / 1000,
pair_trie_root: H64::default(),
base_protocol_info,
rel_protocol_info,
};

let key_pair = ctx.secp256k1_key_pair.or(&&|| panic!());
Expand Down Expand Up @@ -2558,17 +2511,7 @@ pub async fn lp_ordermatch_loop(ctx: MmArc) {
spawn({
let ctx = ctx.clone();
async move {
if let Ok(Some((base, rel))) =
find_pair(&ctx, &maker_order.base, &maker_order.rel).await
{
maker_order_created_p2p_notify(
ctx,
&maker_order,
base.coin_protocol_info(),
rel.coin_protocol_info(),
)
.await;
}
maker_order_created_p2p_notify(ctx, &maker_order).await;
Copy link
Copy Markdown

@artemii235 artemii235 Sep 6, 2021

Choose a reason for hiding this comment

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

@shamardy Could you please clarify, what was the reason to remove if let Ok(Some((base, rel))) = find_pair(&ctx, &maker_order.base, &maker_order.rel).await? It opens a possibility to have an unmatchable maker order in the orderbook unless the coins are activated.

Copy link
Copy Markdown
Collaborator Author

@shamardy shamardy Sep 6, 2021

Choose a reason for hiding this comment

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

I removed it because I added it for maker order protocol info. on this PR #1017 as it wasn't there before. But will add it again to this PR #1054 as we agreed since it appears to have another purpose.

}
});
} else {
Expand Down Expand Up @@ -2661,13 +2604,7 @@ pub async fn lp_ordermatch_loop(ctx: MmArc) {
log::error!("Error {} on subscribing to orderbook topic {}", e, topic);
}
}
maker_order_created_p2p_notify(
ctx.clone(),
order,
base.coin_protocol_info(),
rel.coin_protocol_info(),
)
.await;
maker_order_created_p2p_notify(ctx.clone(), order).await;
}
}
}
Expand Down Expand Up @@ -3108,12 +3045,6 @@ struct OrderbookItem {
min_volume: BigRational,
uuid: Uuid,
created_at: u64,
#[serde(default)]
#[serde(skip_serializing_if = "Option::is_none")]
pub base_protocol_info: Option<Vec<u8>>,
#[serde(default)]
#[serde(skip_serializing_if = "Option::is_none")]
pub rel_protocol_info: Option<Vec<u8>>,
}

#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
Expand Down Expand Up @@ -3614,13 +3545,7 @@ pub async fn set_price(ctx: MmArc, req: Json) -> Result<Response<Vec<u8>>, Strin
let request_orderbook = false;
try_s!(subscribe_to_orderbook_topic(&ctx, &new_order.base, &new_order.rel, request_orderbook).await);
save_my_new_maker_order(&ctx, &new_order);
maker_order_created_p2p_notify(
ctx.clone(),
&new_order,
base_coin.coin_protocol_info(),
rel_coin.coin_protocol_info(),
)
.await;
maker_order_created_p2p_notify(ctx.clone(), &new_order).await;
let rpc_result = MakerOrderForRpc::from(&new_order);
let res = try_s!(json::to_vec(&json!({ "result": rpc_result })));
my_orders.insert(new_order.uuid, new_order);
Expand Down
141 changes: 8 additions & 133 deletions mm2src/lp_ordermatch/best_orders.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use super::{addr_format_from_protocol_info, OrderbookItemWithProof, OrdermatchContext, OrdermatchRequest,
OrdermatchRequestVersion};
use super::{AddressFormat, OrderbookItemWithProof, OrdermatchContext, OrdermatchRequest};
use crate::mm2::lp_network::{request_any_relay, P2PRequest};
use coins::{address_by_coin_conf_and_pubkey_str, coin_conf, is_wallet_only_conf, is_wallet_only_ticker};
use common::log;
Expand Down Expand Up @@ -35,7 +34,6 @@ pub async fn process_best_orders_p2p_request(
coin: String,
action: BestOrdersAction,
required_volume: BigRational,
version: OrdermatchRequestVersion,
) -> Result<Option<Vec<u8>>, String> {
let ordermatch_ctx = OrdermatchContext::from_ctx(&ctx).expect("ordermatch_ctx must exist at this point");
let orderbook = ordermatch_ctx.orderbook.lock().await;
Expand Down Expand Up @@ -65,13 +63,6 @@ pub async fn process_best_orders_p2p_request(
for ordered in orders {
match orderbook.order_set.get(&ordered.uuid) {
Some(o) => {
if version == OrdermatchRequestVersion::V1
&& (o.base_protocol_info.is_some() || o.base_protocol_info.is_some())
{
log::debug!("Order {} address format is not supported by receiver", o.uuid);
continue;
}

let min_volume = match action {
BestOrdersAction::Buy => o.min_volume.clone(),
BestOrdersAction::Sell => &o.min_volume * &o.price,
Expand Down Expand Up @@ -118,7 +109,6 @@ pub async fn best_orders_rpc(ctx: MmArc, req: Json) -> Result<Response<Vec<u8>>,
coin: req.coin,
action: req.action,
volume: req.volume.into(),
version: OrdermatchRequestVersion::V2,
};

let best_orders_res =
Expand All @@ -141,11 +131,13 @@ pub async fn best_orders_rpc(ctx: MmArc, req: Json) -> Result<Response<Vec<u8>>,
}
for order_w_proof in orders_w_proofs {
let order = order_w_proof.order;
let addr_format = match req.action {
BestOrdersAction::Buy => addr_format_from_protocol_info(&order.rel_protocol_info),
BestOrdersAction::Sell => addr_format_from_protocol_info(&order.base_protocol_info),
};
let address = match address_by_coin_conf_and_pubkey_str(&coin, &coin_conf, &order.pubkey, addr_format) {
// Todo: use the right address format when a solution is found for the problem of protocol_info
let address = match address_by_coin_conf_and_pubkey_str(
&coin,
&coin_conf,
&order.pubkey,
AddressFormat::Standard,
) {
Ok(a) => a,
Err(e) => {
log::error!("Error {} getting coin {} address from pubkey {}", e, coin, order.pubkey);
Expand All @@ -166,120 +158,3 @@ pub async fn best_orders_rpc(ctx: MmArc, req: Json) -> Result<Response<Vec<u8>>,
.body(json::to_vec(&res).expect("Serialization failed"))
.map_err(|e| ERRL!("{}", e))
}

#[cfg(test)]
mod best_orders_tests {
use common::new_uuid;
use keys::AddressFormat;
use rmp_serde::decode::Error;
use uuid::Uuid;

use super::super::OrderbookItem;
use super::*;

#[test]
fn check_best_orders_res_deserialize_to_old() {
type TrieProof = Vec<Vec<u8>>;
#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
struct OrderbookItemOld {
pubkey: String,
base: String,
rel: String,
price: BigRational,
max_volume: BigRational,
min_volume: BigRational,
uuid: Uuid,
created_at: u64,
}

#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
struct OrderbookItemWithProofOld {
order: OrderbookItemOld,
last_message_payload: Vec<u8>,
proof: TrieProof,
}

#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)]
struct BestOrdersResOld {
orders: HashMap<String, Vec<OrderbookItemWithProofOld>>,
}

let uuid = new_uuid();

let mut best_order_res_new_orders = HashMap::new();
best_order_res_new_orders.insert(String::from("RICK"), vec![OrderbookItemWithProof {
order: OrderbookItem {
pubkey: "03c6a78589e18b482aea046975e6d0acbdea7bf7dbf04d9d5bd67fda917815e3ed".into(),
base: "tBTC".into(),
rel: "RICK".into(),
price: BigRational::from_integer(1.into()),
max_volume: BigRational::from_integer(2.into()),
min_volume: BigRational::from_integer(1.into()),
uuid,
created_at: 1626959392,
base_protocol_info: None,
rel_protocol_info: None,
},
last_message_payload: vec![],
proof: vec![],
}]);
let best_order_res_new = BestOrdersRes {
orders: best_order_res_new_orders,
};

let mut best_order_res_old_orders = HashMap::new();
best_order_res_old_orders.insert(String::from("RICK"), vec![OrderbookItemWithProofOld {
order: OrderbookItemOld {
pubkey: "03c6a78589e18b482aea046975e6d0acbdea7bf7dbf04d9d5bd67fda917815e3ed".into(),
base: "tBTC".into(),
rel: "RICK".into(),
price: BigRational::from_integer(1.into()),
max_volume: BigRational::from_integer(2.into()),
min_volume: BigRational::from_integer(1.into()),
uuid,
created_at: 1626959392,
},
last_message_payload: vec![],
proof: vec![],
}]);
let best_order_res_old = BestOrdersResOld {
orders: best_order_res_old_orders,
};

// new format should be deserialized to old when protocol_infos are None
let serialized = rmp_serde::to_vec(&best_order_res_new).unwrap();
let deserialized: BestOrdersResOld = rmp_serde::from_read_ref(serialized.as_slice()).unwrap();
assert_eq!(best_order_res_old, deserialized);

// old format should be deserialized to new with protocol_infos as None
let serialized = rmp_serde::to_vec(&best_order_res_old).unwrap();
let deserialized: BestOrdersRes = rmp_serde::from_read_ref(serialized.as_slice()).unwrap();
assert_eq!(best_order_res_new, deserialized);

let mut best_order_res_new_orders = HashMap::new();
best_order_res_new_orders.insert(String::from("RICK"), vec![OrderbookItemWithProof {
order: OrderbookItem {
pubkey: "03c6a78589e18b482aea046975e6d0acbdea7bf7dbf04d9d5bd67fda917815e3ed".into(),
base: "tBTC".into(),
rel: "RICK".into(),
price: BigRational::from_integer(1.into()),
max_volume: BigRational::from_integer(2.into()),
min_volume: BigRational::from_integer(1.into()),
uuid,
created_at: 1626959392,
base_protocol_info: Some(rmp_serde::to_vec(&AddressFormat::Segwit).unwrap()),
rel_protocol_info: Some(rmp_serde::to_vec(&AddressFormat::Standard).unwrap()),
},
last_message_payload: vec![],
proof: vec![],
}]);
let best_order_res_new = BestOrdersRes {
orders: best_order_res_new_orders,
};

// old format can't be deserialized from new when protocol_infos are present
let serialized = rmp_serde::to_vec(&best_order_res_new).unwrap();
let deserialized: Result<BestOrdersResOld, Error> = rmp_serde::from_read_ref(serialized.as_slice());
assert!(deserialized.is_err());
}
}
Loading