GH-744 - Set BlockchainBridge free from blocking caused by RPC calls #456

Syther007 · 2024-07-29T10:06:35Z

No description provided.

This reverts commit ffe6b81.

…ding_nonce

…hour_error_back_to_accountant

…s_finished_payments_back_to_accountant

…ds_finished_payments_back_to_accountant

…ts_back_to_accountant

…_if_no_payments_are_detected

bertllll · 2024-10-23T16:57:46Z

node/src/blockchain/blockchain_bridge.rs

+        let _blockchain_client_server = MBCSBuilder::new(port)
+            .begin_batch()
+            .raw_response(first_response)
+            // A transaction receipt is null when the transaction is not available


If it's not available, it would mean something else than what you think. As long as the pending transaction was added to the transaction pool, it must always be available for an inspection and therefore the response we get back should definitely not say null, but it would hold enough information to form the web3 TransactionReceipt structure. You are supposed to check the field of the structure with name status, if it's None, it means pending, if Some(1), it means a complete transaction, if Some(0), it's a failure.

bertllll · 2024-10-23T17:01:13Z

node/src/blockchain/blockchain_bridge.rs

+            accountant_recording.get_record::<ReportTransactionReceipts>(0);
+        let scan_error_message = accountant_recording.get_record::<ScanError>(1);
+        let mut expected_receipt = TransactionReceipt::default();
+        expected_receipt.transaction_hash = hash_1;


If you use the TransactionReceiptBuilder elsewhere, then you should here too. However, I still hope you will be convinced that it's wrong to use this web3 structure. We need to manipulate our own one, as I explained before.

bertllll · 2024-10-23T17:06:04Z

node/src/blockchain/blockchain_bridge.rs

+              response_skeleton_opt: None,
+              msg: "Error while retrieving transactions: OtherRPCError(\"Attempted to retrieve received payments but failed: QueryFailed(\\\"Transport error: Error(IncompleteMessage)\\\")\")".to_string()
+          }
+        );


Just a reminder. I don't like you send any retrieved transactions and then you send another message saying the scan failed. How can you call it a failure if you actually completed the cycle, even though it wasn't all the transactions?

bertllll · 2024-10-23T17:14:27Z

node/src/blockchain/blockchain_bridge.rs

        TestLogHandler::new().exists_log_containing(
-            "WARN: BlockchainBridge: Attempted to retrieve \
-         received payments but failed: QueryFailed(\"we have no luck\")",
+            "WARN: BlockchainBridge: Error while retrieving transactions: OtherRPCError(\"Attempted to retrieve received payments but failed: QueryFailed(\\\"Transport error: Error(IncompleteMessage)\\\")\")",
        );
    }

    #[test]
    fn handle_request_transaction_receipts_short_circuits_on_failure_from_remote_process_sends_back_all_good_results_and_logs_abort(


You removed the short-circuit functionality, naturally, because you changed the strategy from a sequence to a batch call.

I'm not opposing this factor, it's cool, the problem is, though, there have remained references in the test names to the previous state, for example here, the old name is definitely not up-to-date and you need to think up one anew that will suit the goal of this test.

For you to better understand the old state: "short-circuit" was supposed to describe that if you have a sequence of 8 pending transactions and the fifth one fails on waiting for the response from the blockchain service provider, then we did not try out the sixth or any following ones and just collected 1,2,3,4 and sent it back to Accountant.

JUST NICE TO HAVE

node/src/blockchain/blockchain_bridge.rs

bertllll · 2024-10-23T19:12:16Z

node/src/blockchain/blockchain_bridge.rs

@@ -1185,15 +1462,15 @@ mod tests {
    }

    #[test]
-    fn handle_request_transaction_receipts_short_circuits_on_failure_of_the_first_payment_and_it_sends_a_message_with_empty_vector_and_logs(
-    ) {
+    fn handle_request_transaction_receipts_short_circuits_if_submit_batch_fails() {


Again the wording "short_circuits" is out of place here.

bertllll · 2024-10-23T19:14:51Z

node/src/blockchain/blockchain_bridge.rs

-            .get_transaction_receipt_params(&get_transaction_receipt_params_arc)
-            .get_transaction_receipt_result(Err(BlockchainError::QueryFailed("booga".to_string())));
+        let port = find_free_port();
+        let _blockchain_client_server = MBCSBuilder::new(port).start();


You can add a comment as you did some other tests of this type, explaining that the lack of response from the server will cause the error. I quite like this approach.

Would be nice to have this

bertllll · 2024-10-23T19:46:55Z

node/src/blockchain/blockchain_bridge.rs

@@ -1287,26 +1553,18 @@ mod tests {
                },


I'm commenting on the test "handle_retrieve_transactions_uses_latest_block_number_upon_get_block_number_error".
Can you explain me of what use is this cannibalized test?

Do you know what you did?

You changed the prod code so that there is a method retrieve_transactions in the BlockchainInterface which hosts some code that also includes the incriminated call of get_block_number which is grafted into the lower level interface. You modified it slightly but the test still is using the BlockchainInterfaceMock. Now, isn't there anything suspicious to you having the two piece of information I just put forth to you?

The issue is that if you use a mock, everything out of the guts, will never be called, and you have the method, which is this test supposed to be aimed at, missing this way. So you're testing a situation which you can only get closer to in the test, but you stay away from it.

This what I see makes me think that you only fixed a failing test but didn't think beyond it, about what the test is testing. That's a bit sad finding.

What am I advising? Hmm. It depends on whether you've had already the discussion with Dan about the right and durable design of the BlockchainInterface, because there might be some rearranging.

Otherwise, if you cannot test the condition from this level, but the condition is still there, you have two options.
First, I just recalled why I designed the LowerLevelInt in first place. It was so that I could take the real implementation of the higher level (or main) interface, planning that it will engage in the test but yet one component inside would need to be altered to a mockable object. That's the reason, why the lower level interface has its own trait. So that I could mock it out if I fancied. This was to solve the big problem of testing these complex, layered structures.

Another way is to write this with the BlockchainInterfaceReal and use your MockBlockchainClientServer to invoke an error on the call for get_block_number(). You've done something like this many times already, so you might want to chose the second path by intuition.

Nevertheless, I wonder if the old good purpose of the LowLevelInt and the possibilities of use it offered will be missing. So far, you've succeeded in a substitution of the concept by your tedious way of setting up the MBCServer. Maybe that'll be universal enough also into the future. I hope so.

This is interconnected with a different comment now. So it shouldn't end up in the dust, unless you ignore also the other, parental comment.

bertllll · 2024-10-23T19:52:06Z

node/src/blockchain/blockchain_bridge.rs

+        let accountant_addr =
+            accountant.system_stop_conditions(match_every_type_id!(ReceivedPayments));
+        let earning_wallet = make_wallet("earning_wallet");
+        let amount = 996000000;
        let expected_transactions = RetrievedBlockchainTransactions {


This structure should be placed after the act, in the assert area.

bertllll · 2024-10-23T20:37:25Z

node/src/blockchain/blockchain_bridge.rs

-                    wei_amount: amount2,
-                },
-            ],
+            new_start_block: 1000000001,


If this is composed as 0x3B9ACA00 (the single response from your mock server) than maybe prepare the expected value as 0x3B9ACA00 + 1

But you could also pick out a better-looking value. I know this is derived from the decimal being nice for eyes, but let's regard it only from the hexadecimal outlook.

bertllll · 2024-10-23T20:43:09Z

node/src/blockchain/blockchain_bridge.rs

+        let system = System::new(test_name);
+        let logger = Logger::new(test_name);
+        let port = find_free_port();
+        let expected_response_logs = vec![LogObject {


Can you please mark the lines with the invalid topics by comments?

Would be nice to have.

bertllll · 2024-10-23T20:45:26Z

node/src/blockchain/blockchain_bridge.rs

+        subject.received_payments_subs_opt = Some(accountant_addr.clone().recipient());
+        subject.scan_error_subs_opt = Some(accountant_addr.recipient());
+        subject.handle_scan_future(
+            BlockchainBridge::handle_retrieve_transactions,
            ScanType::Receivables,
            retrieve_transactions,
        );


Isolate the act.

node/src/blockchain/blockchain_bridge.rs

bertllll · 2024-10-23T21:00:10Z

node/src/blockchain/blockchain_bridge.rs

-            }
-        );
-        assert_eq!(accountant_recording.len(), 1);
-        TestLogHandler::new().exists_log_containing("WARN: BlockchainBridge: My tummy hurts");


Maybe you want to add a log assertion with "exists_no_log_containing" to prove that we did not deliver an error to these places and so it didn't have to mention it. The message should actually also name the logger at the very beginning of it, which would be the name of test itself.

Would be nice to have.

bertllll · 2024-10-23T21:42:53Z

node/src/blockchain/blockchain_bridge.rs

+        });
+    }
+
+    fn assert_handle_scan_future_handles_failure(msg: RetrieveTransactions) {


Oh guys...I cannot drag it on anymore... you have the understanding of these tests about the handle_scan or handle_scan_futures somewhat screwed.

Especially, there are supposed to be two tests which you conceded into just one. You missed out the important difference. It touches the ResponseSkeleton. You need to ensure that the ResponseSkeleton, if it is Some - and therefore present, needs to be take over by the ScanErrorMsg.

Because you have only one test for the error situation it's easy to deduce that you lack the second test exercising the flight of the ScanErrorMsg when the skeleton hasn't been populated from the beginning (meaning that the scan wasn't ordered manually by the command intended for it).

It also explains for you why you ended up with this strangely ostracized assert_handle_scan_future_handles_failure. It came to existence only because it was to be shared between two tests whose inputs differ only minimally.

URGENCY STATUS: THIS IS A CASE OF CRIPPLING THE OLD TESTS, AND WE CANNOT TOLERATE LOSING THE TEST COVERAGE BECAUSE WE ARE IN HURRY. PLEASE FIX IT SO THAT BOTH SITUATIONS ARE TESTED See the explanation above.

Hi Bert,

Could you please point to the test that would test this ScanErrorMsg when the ResponseSkeleton is Some (such that two request were generated).
We have checked Master and struggled to find a test that related to these tests and also tested ScanErrorMsg

Regarding the handle_scan method appropriated to the BlockchainBridge, three tests makes up the full coverage on the master branch.

Those three are:
handle_scan_handles_success (the happy path test)
handle_scan_handles_failure_without_skeleton (a general failure while the skeleton is not there)
handle_scan_handles_failure_with_skeleton (a general failure while the skeleton was provided)

Now, how can you understand the skeleton? It's a construct which actually represents a request from a UI. It stores the two numbers that are important for a continued communication: client_id (represents the UI), and the context_id (represents the specific conversation).

You miss the second one. I recommend to try grafting onto this method assert_handle_scan_future_handles_failure and you might find yourselves with two tests sharing this function as a test body thanks to which it will save some space.

Hi Bert,

I'm a little confused by your comment.

It appears we are still testing all 3 cases.
We have created a helper function assert_handle_scan_future_handles_failure to help test both cases for the skeleton using test handle_scan_future_handles_failure.

Here the tests code.

#[test] fn handle_scan_future_handles_failure() { assert_handle_scan_future_handles_failure(RetrieveTransactions { recipient: make_wallet("somewallet"), response_skeleton_opt: Some(ResponseSkeleton { client_id: 1234, context_id: 4321, }), }); assert_handle_scan_future_handles_failure(RetrieveTransactions { recipient: make_wallet("somewallet"), response_skeleton_opt: None, }); }

bertllll · 2024-10-23T21:52:52Z

node/src/blockchain/blockchain_bridge.rs

@@ -1811,117 +2052,4 @@ pub mod exportable_test_parts {
            )
        }
    }


One opinion is, remembering instructing you to go ahead and delete the test this was done for, it can simply be erased along with the test.

Otherwise:
If this is the only impl that has remained since your first run at this card, let me tell you that it doesn't require exclusive public module making a circle around it. Just grab it and move it to the tests module that's used in all other cases.

Please do it

node/src/blockchain/blockchain_bridge.rs

bertllll · 2024-10-23T22:03:34Z

node/src/blockchain/blockchain_bridge.rs

+                1,
+            )
+            .start();
+


Blank line at a wrong place.

* GH-813: Correctly parse max block range error message (#531) * GH-500: Adding Base chains (#510) * GH-500: ready for an urged live test * GH-500: probably finished the deployment of Base, let's get the QA going * GH-500: removed an eprintln! * GH-500: version changed to 0.8.1 --------- Co-authored-by: Bert <[email protected]> * update readme and tag v0.8.1 (#532) Signed-off-by: KauriHero <[email protected]> * GH-524: Disable `entry_dns` (#526) * GH-539: Don't Panic! (#540) * GH-539: Don't Panic * GH-539: remove commented out code * New Version: v0.8.2 (#542) * GH-744: Review-1 first lot of changes * GH-744: Review-1 - fixed more tests * GH-744: improved wildcard IP check * GH-744: removed unused imports * GH-744: fixed a bunch more comments from review 1 * GH-744: resolved more review comments * GH-744: started converting gas_price units from gwei to wei * GH-744: finish converting gas price from gwei to wei * GH-744: Formating & removed warnings * GH-744: Added TransactionFailed to TransactionReceiptResult * GH-606: Initialize start_block to none to use latest block (#374) * GH-606: Initialize start_block to none to use latest block * GH-606: Apply PR feedback changes * GH-606: Apply PR feedback changes * GH-606: Apply PR feedback changes * GH-606: Apply PR review 4 feedback changes * GH-606: Squashing commits - Save start_block_nbr if no msg but send as non-Option - Always commit - Reduce logging levels and simplify - Follow the Option naming pattern * GH-600: set_start_block only called in accountant/scanners/mod.rs * GH-606: PR Feedback - parameterize a test * GH-606: Address PR feedback * GH-606: Implement parameterized test without crate macro * GH-744: Migrated the guts of get_transaction_receipt_in_batch to process_transaction_receipts * GH-744: Moved submit_payables_in_batch to blockchain_interface * GH-744: removed test: blockchain_bridge_can_return_report_transaction_receipts_with_an_empty_vector * GH-744: Fixed a few more URGENCY comments * GH-744: cleanup & formatting * GH-744: add some TODOs as discussed on Wed and Thu * GH-744: handle_request_transaction_receipts chaged error to a DEBUG log * GH-744: handle_retrieve_transactions inbeded extract_max_block_count * GH-744: code refactor * GH-744: removed transaction_id from Agent * GH-744: Removed get_gas_price from submit_batch call * GH-744: logger is now a reference in send_payables_within_batch * GH-744: send_payables_within_batch web3_batch is now a reference * GH-744: sign_and_append_multiple_payments accounts is now a reference * GH-744 removed Blockchan_interface_mock * GH-744: Refactored all 4 test for send_payables_within_batch * GH-744: cleanup & formatting * GH-744: small fixs * GH-744: handle_normal_client_data detects wildcard IP & localhost with error * GH-744: Finished all urgent comments * GH-744: changed actions macos-12 to 13 * GH-744: increased sleep time for test provided_and_consumed_services_are_recorded_in_databases * GH-744: Fixed multinode tests * GH-744: Added start block +1 to handle_transaction_logs * GH-744: Fixed some tests * GH-744: Fixes from review 2 * GH-744: Fixed test debtors_are_credited_once_but_not_twice * GH-744: removed BlockNumber::Number * GH-744: Resolved TODOs * GH-744: First commit for review-3, fixed tests * GH-744: Refactored TxReceipt * GH-744: Refactored TxResponse * GH-744 moved & renamed blockchain_interface_utils.rs * GH-744: fixed test: dns_resolution_failure_for_wildcard_ip_with_real_nodes * GH-744: add review 4 changes * GH-744: add review 5 changes * GH-744: remove the map_err() * GH-744: migrate the logging code to a different function * GH-744: add review 6 changes --------- Signed-off-by: KauriHero <[email protected]> Co-authored-by: MASQrauder <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: Bert <[email protected]> Co-authored-by: KauriHero <[email protected]> Co-authored-by: Syther007 <[email protected]>

…lient_stream_is_killed_integration

…nodes

This reverts commit f3d8315.

Syther007 added 30 commits September 27, 2023 23:54

Added base comments

e908666

GH-744: removed .wait() from handle_request_balances_to_pay_payables

8b71081

GH-744: added future return type to get_transaction_fee_balance

99e36a5

Fixed error mapping for .get_token_balance

4fba6f1

GH-744: Fixed failing tests, all tests passing

7d0d075

GH-744: Added future return to ReportAccountsPayable

159ad22

GH-744: Fixed more errors

5b25bce

GH-744: Added batch lifetime

ffe6b81

Revert "GH-744: Added batch lifetime"

cb2f3e4

This reverts commit ffe6b81.

GH-744: Fixed more errors

e5482f7

GH-744: small clean up

8093caf

GH-744: Moved batch_payable_tools functions

85529d0

GH-744: send_payables_within_batch accounts changed to Vec

aa811db

GH-744: added blockchain_interface_slim

d1659ec

Added blockchain_interface_utils.js

0877056

GH-744: process_payments is working with futures

a075b5d

GH-744: Fixed a few more tests in blockchain_interface_utils

70ee731

GH-744: Fixed test useing test_gas_limit_is_between_limits

2f03dbe

Changed sign_transaction_data amount data type to u128

ba6a7f9

GH-744 converted handle_retrieve_transactions return type to future

c7725b1

GH-744: Fixed some error with handle_retrieve_transactions

2a16982

GH-744: update_start_block_message_works test works

93189ec

GH-744: Added get_batch_web3 Mock

231cce3

GH-744: Fixed test report_accounts_payable_returns_error_fetching_pen…

25593c5

…ding_nonce

GH-744: fixed test process_payments_returns_error_from_sending_batch

c313533

GH-744: fixed test handle_report_accounts_payable_transmits_eleventh_…

e771cd5

…hour_error_back_to_accountant

GH-744: Edited test handle_report_accounts_payable_transacts_and_send…

d3a3694

…s_finished_payments_back_to_accountant

GH-744: updated test handle_report_accounts_payable_transacts_and_sen…

6c6a146

…ds_finished_payments_back_to_accountant

GH-744: FIxed test handle_retrieve_transactions_sends_received_paymen…

e645549

…ts_back_to_accountant

GH-744: Update to test processing_of_received_payments_continues_even…

5e1dac1

…_if_no_payments_are_detected

bertllll reviewed Oct 23, 2024

View reviewed changes

kauri-hero mentioned this pull request Nov 17, 2024

Streamline Messages from BlockchainBridge to the Accountant #557

Open

utkarshg6 and others added 24 commits January 6, 2025 17:54

Merge branch 'master' into GH-744

f0725e3

GH-744: an attempt to fix test proxy_client_stream_reader_dies_when_c…

a02c83a

…lient_stream_is_killed_integration

GH-744: add TODO in validate_ipv4 and make the unit tests passing

e8c9324

GH-744: Fixed test: debtors_are_credited_once_but_not_twice

835b7ac

GH-744: Fixed test: dns_resolution_failure_for_wildcard_ip_with_real_…

03cf21f

…nodes

GH-744: fixed test blockchain_bridge_subs_factory_produces_proper_subs

ffab247

GH-744: Added temp trace logs for Debugging

8814611

GH-744: Fixed Clippy

83137c6

GH-744: changed the way we handle start_block updates

f3d8315

Revert "GH-744: changed the way we handle start_block updates"

12ab802

This reverts commit f3d8315.

GH-744: added more debugging for process_transaction_receipts

b265279

GH-744: debugging for get_transaction_receipt_in_batch

65d5a85

GH-744: added multi node test verify_blockchain_payments

8fee838

GH-744: improved Test verify_blockchain_payments

98040e3

GH-744: fixed get_transaction_receipt_in_batch

cc11cab

GH-744: Fixed start block Latest bug

43571b7

GH-744 log cleanup

332938e

GH-744: fixes for QA Review

97e9c73

GH-744: improved is_running_in_integration_test as per review

c8d4d6b

GH-744: clean up

7c765c7

Gh-744: refactored retrieve_transactions for Start & End BlockNumber

39ff5ce

GH-744: Fixes in regards to QA-Review-2

67fd716

GH-744: final review changes

a6a78ce

bertllll approved these changes Feb 14, 2025

View reviewed changes

Syther007 merged commit c549912 into master Feb 15, 2025
6 checks passed

kauri-hero mentioned this pull request Feb 16, 2025

Set BlockchainBridge free from blocking caused by RPC calls MASQ-Project/MASQ-Node-issues#744

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-744 - Set BlockchainBridge free from blocking caused by RPC calls #456

GH-744 - Set BlockchainBridge free from blocking caused by RPC calls #456

Syther007 commented Jul 29, 2024

bertllll Oct 23, 2024

bertllll Oct 23, 2024

bertllll Oct 23, 2024 •

edited

Loading

bertllll Oct 23, 2024 •

edited

Loading

bertllll Oct 30, 2024

bertllll Oct 23, 2024

bertllll Oct 23, 2024

bertllll Oct 30, 2024

bertllll Oct 23, 2024 •

edited

Loading

bertllll Oct 30, 2024

bertllll Oct 23, 2024

bertllll Oct 23, 2024

bertllll Oct 23, 2024

bertllll Oct 30, 2024

bertllll Oct 23, 2024

bertllll Oct 23, 2024

bertllll Oct 30, 2024

bertllll Oct 23, 2024

bertllll Oct 30, 2024

Syther007 Nov 4, 2024

bertllll Nov 17, 2024

bertllll Nov 17, 2024

Syther007 Nov 18, 2024

bertllll Oct 23, 2024 •

edited

Loading

bertllll Oct 30, 2024

bertllll Oct 23, 2024

@@ @@ -1811,117 +2052,4 @@ pub mod exportable_test_parts { @@
                           )
                       }
                   }

GH-744 - Set BlockchainBridge free from blocking caused by RPC calls #456

GH-744 - Set BlockchainBridge free from blocking caused by RPC calls #456

Conversation

Syther007 commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

bertllll Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertllll Oct 23, 2024 •

edited

Loading

bertllll Oct 23, 2024 •

edited

Loading

bertllll Oct 23, 2024 •

edited

Loading

bertllll Oct 23, 2024 •

edited

Loading