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

Jump to bottom

feat: Configurable sync_notifs backoff #3278

Merged

nieznanysprawiciel merged 9 commits into master from mw/configurable-sync-notifs-backoff

Jul 29, 2024

Collaborator

mwalkiewicz commented Jul 23, 2024 •

edited by nieznanysprawiciel

Loading

resolves: golemfactory/gamerhash-facade#183
scenarios: https://github.com/golemfactory/gamerhash-facade/wiki/Payments-sync-testing-scenarios

mwalkiewicz requested a review from a team as a code owner

July 23, 2024 14:24

CLAassistant commented Jul 23, 2024 •

edited

Loading

All committers have signed the CLA.

nieznanysprawiciel previously approved these changes

View reviewed changes

core/payment/src/lib.rs Show resolved Hide resolved

mwalkiewicz dismissed nieznanysprawiciel’s stale review via

69dede0

July 25, 2024 18:41

mwalkiewicz force-pushed the mw/configurable-sync-notifs-backoff branch 4 times, most recently from 8e0fc8f to 22ad437 Compare

July 25, 2024 20:38


          feat: Configurable sync_notifs backoff

6f70a63

mwalkiewicz force-pushed the mw/configurable-sync-notifs-backoff branch 2 times, most recently from 1c0d514 to 1f8008c Compare

July 25, 2024 21:34

mwalkiewicz added 4 commits

July 25, 2024 23:36


          fix: correctly select and send payment and invoices syncs

7d31c58


          Feat: implement PaymentSyncWithBytes handling

0c02456


          fix: add sync entry for receiver instead of sender for delayed debit …

5a166bf

…notes accepts


          fix: Use full primary key when selecting payments for marking as sent

962c28f

mwalkiewicz force-pushed the mw/configurable-sync-notifs-backoff branch from 1f8008c to 962c28f Compare

July 25, 2024 21:37

nieznanysprawiciel reviewed

View reviewed changes

core/payment/src/dao/payment.rs Outdated

Comment on lines 162 to 170

+                  pub async fn mark_sent(&self, payment_id: String, owner_id: NodeId) -> DbResult<()> {
                       do_with_transaction(self.pool, "payment_dao_mark_sent", move |conn| {
-                          diesel::update(dsl::pay_payment.filter(dsl::id.eq(payment_id)))
-                              .filter(dsl::role.eq(Role::Requestor))
-                              .set(dsl::send_payment.eq(false))
-                              .execute(conn)?;
+                          diesel::update(
+                              dsl::pay_payment.filter(dsl::id.eq(payment_id).and(dsl::owner_id.eq(owner_id))),
+                          )
+                          .filter(dsl::role.eq(Role::Requestor))
+                          .set(dsl::send_payment.eq(false))
+                          .execute(conn)?;
                           Ok(())

Contributor

nieznanysprawiciel Jul 26, 2024

The previous version was correct, since payment_id and role should be unique. Your version should be correct as well, but I would restore previous, for 2 reasons:

To keep function signature more pleasant
To avoid risk of breaking something

core/payment/src/payment_sync.rs Outdated

Comment on lines 34 to 47

+              fn remove_allocation_ids_from_payment(payment: &Payment) -> Payment {
+                  // We remove allocation ID from syncs because allocations are not transferred to peers and
+                  // their IDs would be unknown to the recipient.
+                  let mut payment = payment.clone();
+                  for agreement_payment in &mut payment.agreement_payments.iter_mut() {
+                      agreement_payment.allocation_id = None;
+                  }
+                  for activity_payment in &mut payment.activity_payments.iter_mut() {
+                      activity_payment.allocation_id = None;
+                  }
+                  payment
+              }

Contributor

nieznanysprawiciel Jul 26, 2024

This is the same functionality as

yagna/core/payment/src/processor.rs

Lines 493 to 499 in 09ee56f

    
           // Allocation IDs are requestor's private matter and should not be sent to provider 
        
           for agreement_payment in payment.agreement_payments.iter_mut() { 
        
               agreement_payment.allocation_id = None; 
        
           } 
        
           for activity_payment in payment.activity_payments.iter_mut() { 
        
               activity_payment.allocation_id = None; 
        
           }

so should be handled by the same code.

Payment struct is defined in ya-client, so I wouldn't modify it there, but probably you could define trait with function remove_private_info or something like that and implement this trait for Payment.

Collaborator Author

mwalkiewicz Jul 29, 2024

Good catch, I'll reuse the code. However I'd refrain from creating a trait (an abstraction) for a single use case and stick with a free function in core/payment/utils.rs if this is OK for you.

core/payment/src/payment_sync.rs Outdated

@@ @@ -45,6 +62,8 @@ async fn payment_sync( @@
                       let platform_components = payment.payment_platform.split('-').collect::<Vec<_>>();
                       let driver = &platform_components[0];
+                      let payment = remove_allocation_ids_from_payment(&payment);
                       let signature = typed::service(driver_bus_id(driver))

Contributor

nieznanysprawiciel Jul 26, 2024

We could query signature from database probably and sign only if we need to rely on fallback.
But this code was already implemented this way, so let's leave it as is

core/payment/src/payment_sync.rs

                       let mut result = ya_net::from(default_identity)
                           .to(peer)
                           .service(ya_core_model::payment::public::BUS_ID)
                           .call(msg_with_bytes.clone())
                           .await;
-                      if matches!(&result, Err(Error::GsbBadRequest(_))) {

Contributor

nieznanysprawiciel Jul 26, 2024

You comment made me realize, that those mechanisms broke because of transition to central net.
We shouldn't remove previous version, because we will break hybrid net this way

core/payment/src/payment_sync.rs Outdated

+                          secs
+                      };
+                      capped
+                  };
                   let cutoff = Utc::now();
                   let default_identity = typed::service(identity::BUS_ID)

Contributor

nieznanysprawiciel Jul 26, 2024

You can list identities instead of taking default. Check identity::List

nieznanysprawiciel reviewed

View reviewed changes

core/payment/src/payment_sync.rs Outdated

               async fn payment_sync(
                   db: &DbExecutor,
+                  current_node_id: NodeId,

Contributor

nieznanysprawiciel Jul 26, 2024

I would call it current. Not sure what it means in this context. Maybe just node_id or owner_id`

mwalkiewicz added 4 commits

July 29, 2024 11:32


          PR fixes

7c87c6c


          PR fixes 2

d30a5de


          pr fix 3

3d853e6


          pr fix 4

a084d67

nieznanysprawiciel approved these changes

View reviewed changes

nieznanysprawiciel merged commit 83d4600 into master

35 checks passed

nieznanysprawiciel deleted the mw/configurable-sync-notifs-backoff branch

July 29, 2024 11:29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet