GH-606: Initialize start_block to none to use latest block #374

masqrauder · 2024-01-21T21:01:02Z

No description provided.

bertllll

There is an important comment at the beginning of the file "migration_9_to_10.rs".

bertllll · 2024-01-23T04:25:39Z

masq/src/commands/set_configuration_command.rs

+    } else {
+        match start_block.parse::<u64>() {
+            Ok(_) => Ok(()),
+            _ => Err(start_block),


I'd like to see a longer err message that would contain also either the parse err or an assistive instruction that it needs to be a valid integer.

bertllll · 2024-01-23T04:29:11Z

masq/src/commands/set_configuration_command.rs

@@ -126,6 +130,7 @@ mod tests {
    fn validate_start_block_works() {


This is actually a bad test because if the function returns an err we want to know if it is an appropriate one, or its message meaningful.

I'd like to see two-sides assertion with Ok(()) or Err("The error message").

I think this now does what you mean... 2 separate test methods one for Err cases and this one for Ok cases.

bertllll · 2024-01-23T04:41:38Z

node/src/blockchain/blockchain_bridge.rs

-            Err (e) => panic! ("Cannot retrieve start block from database; payments to you may not be processed: {:?}", e)
+            Ok(Some(sb)) => sb,
+            Ok(None) => u64::MAX,
+            Err(e) => panic!("Cannot retrieve start block from database; payments to you may not be processed: {:?}", e)
        };
        let max_block_count = match self.persistent_config.max_block_count() {
            Ok(Some(mbc)) => mbc,


I'm having a hard time to accept that we ignore the error. I know it isn't dangerous if we don't have it, on the other hand, at all places I remember we treat such an error from our communication with the database manager, specifically, we always go and panic.

An argument is, though, that there is a card that should make this reaction implicit, ripping the Result structure from all the methods of PersistentConfiguration. Now, advice is worth gold. I don't feel like needing to insist on this.

bertllll · 2024-01-23T04:46:40Z

node/src/blockchain/blockchain_bridge.rs

                if max_block_count == u64::MAX {
+                    info!(
+                        self.logger,
+                        "Using 'latest' block number instead of a literal number. {:?}", e


You first need to think about what to do with the error. Please don't just throw it away. I think logging would be fair, we do it all the time.

The way from here, I think, goes by dividing this message into two logs (or even three, according to the differences I mentioned about the if statement). One for the error, maybe perceived as WARN and another less sever with this little informative message (or actually two like that).

If you want to keep an info log, which means one that "we want the user to see", it maybe should have more information to give so that they understand the context. Like what bigger operation is going on of which this is a smaller part.

Remember there is a rule that all info logs will eventually display to the user directly in the UI, not just write to the file.

I do think this kind of information is useful for us developers rather than anybody else so I'd choose just a debug log instead (or actually two debug logs). The fine think about this is you wouldn't have to make assertions on them.

bertllll · 2024-01-23T04:49:56Z

node/src/blockchain/blockchain_bridge.rs

@@ -314,25 +315,31 @@ impl BlockchainBridge {
            .get_block_number()
        {
            Ok(eb) => {
-                if u64::MAX == max_block_count {
+                if u64::MAX == max_block_count || u64::MAX == start_block_nbr {


You use this boolean condition a couple of times again and again. Maybe you could create a variable before here, with a nice name, that would store the resolution of this expression. Then use the variable where you need to.

I think I'd find it easier to understand since I wouldn't have to check if those are different conditions or the same one all the time.

bertllll · 2024-01-23T10:17:29Z

node/src/database/config_dumper.rs

+    fn assert_none(key: &str, map: &Map<String, Value>) {
+        assert!(!map.get(key).is_none());
+    }
+


I have two notes on this.

First, I somewhat cannot believe this assertion stands right. I think it tests something completely different than what the name of this helper function tries to convey.

It only tests that "a certain parameter (and its row) IS present in the db". That is far from what you intended to get.

Let me suggest a solution:
You should simply reach the value from the set by using 'get(key)' but don't stop there, now you got an enum with variants representing possible values that can take part in a JSON. Look at the body of "assert_value" above your function. Do you see 'Value::String()'? That's one of the variants. Another one is 'Value::Null'. Your assertion should only make sure that you get exactly this Value when you ask for your parameter.

bertllll · 2024-01-23T10:20:08Z

node/src/node_configurator/configurator.rs

+        assert_eq!(*check_start_block_params, vec![Some(166666)]);
+        TestLogHandler::new().exists_log_containing(&format!(
+            "DEBUG: {}: A request from UI received: {:?} from context id: {}",
+            test_name, msg, context_id


We try not to assert on Debug logs 🤷‍♂️

bertllll · 2024-01-23T10:36:53Z

node/src/node_configurator/configurator.rs

@@ -549,7 +549,8 @@ impl Configurator {
            persistent_config.earning_wallet_address(),
            "earningWalletAddressOpt",
        )?;
-        let start_block = Self::value_required(persistent_config.start_block(), "startBlock")?;
+        let start_block_opt =
+            Self::value_not_required(persistent_config.start_block(), "startBlock")?;


You haven't provided any assertion for this where the value was None.

Is this where that is checked or do I need something more?: https://github.com/MASQ-Project/Node/blob/GH-606/node/src/node_configurator/configurator.rs#L2090

bertllll · 2024-01-23T10:47:02Z

node/src/node_configurator/configurator.rs

-        let block_number = match string_number.parse::<u64>() {
-            Ok(num) => num,
-            Err(e) => return Err((NON_PARSABLE_VALUE, format!("start block: {:?}", e))),
+        let block_number_opt = if "none".eq_ignore_ascii_case(&string_number) {


I'm sorry to be adding in more work for you but I need to be thorough. I'll bet the use of this function (from a library) isn't tested.

Once you use something that can behave variously or produce different outcomes you will need to test it separately.

I recommend to test this small function only, on values like "noNe, none, NONE", always passing the same. Using a PersistentConfigurationMock and assertions on the params of set_start_block(), making sure it is supplied by the 'None' value properly.

eq_ignore_ascii_case() looks to me like it's part of the Rust standard library, isn't it? Normally I try not to write tests that only test external library code already having its own tests. Do you agree?

I agree with your take on this but it isn't really addressing what I was poking into. I can go along with you that sometimes my opinions are extreme, as well as here: I just pointed out in my old comment that your only test doesn't cover the behaviour which was added by incorporating the eq_ignore_ascii_case. It really is extreme because my reasoning stem from "if somebody went and rewrote the place with some different code, yet accepting "none", there is no guarantee it would still be code supporting also the other letter-casing. Here it is slightly hilarious, but it raises the question whether it is important that this extra little functionality is in there.

Okay, restating myself: my concern was that the test cannot tell apart, if the production code uses

let block_number_opt = if "none".eq_ignore_ascii_case(&string_number)...

or simply

let block_number_opt = if string_number == "none" {...

You see what I mean?

bertllll · 2024-01-23T11:48:27Z

node/src/database/db_migrations/migrations/migration_9_to_10.rs

@@ -0,0 +1,71 @@
+use crate::database::db_migrations::db_migrator::DatabaseMigration;
+use crate::database::db_migrations::migrator_utils::DBMigDeclarator;
+


Ohhhhhhh. This took me a good amount of time before I realised. That what I want to tell you is probably not going to be good news to you.

It will affect a lot of comments that I'd made before this moment.

Thinking of schema of the database, you must admit that with your card there has been no change. Not even a fingernail.

The data types in the columns for the config table and the values of the parameters have ever been declared as an optional TEXT.

Which is exactly what you are using.

Now, let's thing about what older databases could have had in their row for the start_block. Well, it was always a number. (And even if there was a way how it could yield 'Null', it would still be completely in accordance with your design.

That means you don't need any db migration at all. For that reason, you should make sure all the related code will go away. I'm sorry.

Please don't forget delete this whole file.

masqrauder · 2024-09-24T00:38:24Z

node/src/accountant/scanners/mod.rs

+                match txn.commit() {
+                    Ok(_) => debug!(logger, "The new_start_block ({}) is not greater than the current_start_block ({}). \
+                    Committed received transactions.", &new_start_block, &current_start_block),
+                    Err(e) => panic!("Commit of received transactions failed: {:?}", e),
+                }
+            }
+        } else {
+            match txn.commit() {
+                Ok(_) => debug!(logger, "Committed received transactions."),
+                Err(e) => panic!("Commit of received transactions failed: {:?}", e),


These are the additional 2 paths that needed the txn.commit() calls

bertllll · 2024-09-24T20:35:45Z

multinode_integration_tests/docker/blockchain/entrypoint.sh

+    -p 18545 \
+    --networkId 2 \
+    --verbose \
+    --mnemonic "timber cage wide hawk phone shaft pattern movie army dizzy hen tackle lamp absent write kind term toddler sphere ripple idle dragon curious hold"


I'm not consistent on how we should finish this branch regarding these files in the docker folder.

Do you think it is not reasonable to put the master alike code in here? Especially the comment speaks about things that cannot be found on your branch, instead the GH-711 has it. Deleting the comment is necessary, therefore I'm suggesting to undo the changes in both files:

multinode_integration_tests/docker/blockchain/entrypoint.sh multinode_integration_tests/docker/blockchain/Dockerfile

Master does not have -h 0.0.0.0 however BlockchainServer::wait_until_ready(); watches for 10 seconds to find it with output.contains("Listening on 0.0.0.0:18545"). It seems right to be explicit about something the test relies on to succeed. But I can revert it if you prefer.

No probs. I'm no longer concerned about this. You can leave it in if you want to.

bertllll · 2024-09-24T21:15:20Z

multinode_integration_tests/src/masq_real_node.rs

@@ -1018,7 +1018,7 @@ impl MASQRealNode {

        args.push("test_node_image");
        let mut command = Command::new("docker", Command::strings(args));
-        command.stdout_or_stderr()?;
+        println!("{}", command.stdout_or_stderr()?);


I'm unsure if we want to keep this debugging feature? I was not the person using it so I have a limited picture of how useful it could be. But it makes the impression on me that it was intended as temporary.

Could you either cultivate it somehow, maybe with a literal part of the final string introducing what the printed stuff is about? Or simply do a deletion.

I think RustRover was complaining that it was an unused result. I figured this was harmless in test code.

bertllll · 2024-09-24T21:24:01Z

multinode_integration_tests/src/mock_blockchain_client_server.rs

+                    if !responses.is_empty() {
+                        let response = responses.remove(0);
+                        Self::send_body(conn_state, response);
+                    }


Are you sure this is something we want to have? I don't think so. Let me check if I understand what is going on here: this is a mock server which is supposed to serve requests as if sent to a real blockchain service. And I do think each such a test should also contain the second part, where it awaits a response. The response needs to be set up in the mock before the actual test starts off, if this step is left out it is actually an error of the programmer and therefore it's adequate to halt the test if we happen to find out there's been a request but we can't reach in for a prepared response. That's actually this very moment which you bypassed with the extra if statement.

Correct me if I can't see the improvement beyond your modification. However, unless you do, I'd definitely revert these lines.

I only changed this because responses was empty and causing a panic. I thought it was hiding the real test failure. I can revert it if you prefer.

bertllll · 2024-09-24T21:26:25Z

multinode_integration_tests/src/mock_blockchain_client_server.rs

            "POST / HTTP/1.1\r\ncontent-type: application/json\r\nuser-agent: web3.rs\r\nhost: 172.18.0.1:32768\r\ncontent-length: 308\r\n\r\n{\"jsonrpc\":\"2.0\",\"method\":\"eth_getLogs\",\"params\":[{\"address\":\"0x59882e4a8f5d24643d4dda422922a870f1b3e664\",\"fromBlock\":\"0x3e8\",\"toBlock\":\"latest\",\"topics\":[\"0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef\",null,\"0x00000000000000000000000027d9a2ac83b493f88ce9b4532edcf74e95b9788d\"]}],\"id\":0}".to_string()
        ])
    }

    fn connect(port: u16) -> TcpStream {
-        let deadline = Instant::now().add(Duration::from_secs(1));
+        let deadline = Instant::now().add(Duration::from_secs(5));


Is the increase necessary? Hmm. I think it's unimportant and so I'd rather go back on this. I can see why it could be 1500 millis or 2000 millis perhaps but 5 seconds seems weird.

I can revert this. I may have been trying to resolve macOS multinode failures that were hopeless to solve here.

bertllll · 2024-09-24T21:33:39Z

multinode_integration_tests/tests/verify_bill_payment.rs

@@ -435,7 +432,7 @@ fn expire_payables(path: PathBuf) {
    statement.execute([]).unwrap();

    let mut config_stmt = conn
-        .prepare("update config set value = '0' where name = 'start_block'")
+        .prepare("update config set value = '0' where name = 'start_block' and (value is null or value < 1)")


Is the part saying value < 1 really important to have? I feel like we don't have to worry there. Even hypothetically, if it was negative it would be good to check if it kills the Node. Just my thoughts, however, I'll leave this up to you. This doesn't bother me in particular.

I'll revert it. It was a fleeting thought to use the same thing in production code to prevent undoing progress by storing a smaller number for the config value. Then I realized we don't have the ability to have a custom where clause to it.

multinode_integration_tests/tests/verify_bill_payment.rs

bertllll · 2024-09-24T22:17:51Z

node/src/accountant/scanners/mod.rs

@@ -938,13 +939,20 @@ impl ReceivableScanner {
                    ),
                }
                match txn.commit() {
-                    Ok(_) => {


Nope. You did not understand my objections. I cannot comment on the exact line but I will leave it here.

You must not have an operation setting the start_block at any other place than here and only here, that is, after making sure any inbound payments are properly recorded. Now go and look into your code in node/src/blockchain/blockchain_bridge.rs at the method handle_retrieve_transactions. You'll see what I'm telling is not desirable. You must throw away the set_start_block functionality. Which will hopefully also affect some tests.

However that's not all, after it, you need to take care of this area in turn. I noticed that the actor message which introduces the operations in this big function, has the field new_start_block declared as an Option of u64. Please check out the code in blockchain_bridge and look for where you feed this field with values. There is just one place and it always uses Some() wrapping the actual block number around. It's constantly in use. That's apparently wrong.

We can reason that at the time when we're constructing the ReceivedPayments msg there we must already have the start_block defined, which is implied by the preceding operations in there (that you incorporate by one of your past cards). All I'm saying is that field does not need to be optional. However, I'm tired to death at the moment and I'd want you to double-check if I did the survey right.

Once we have an agreement about it, we need to move over here and fix it so that if let Some(new_start_block) = msg.new_start_block { makes sense in this new perspective. The field is not optional anymore. So what can we do? Basically, we've got rid off one of the else clauses and you can erase the lines 953-955. That's great news.

I agree setting the start block number in a single place is the right way to go.

Where is the single best place for calling set_start_block? Let me ask a few questions to show my thought process to arrive at blockchain_bridge as the correct answer.

Which impl 'knows' the most about the block number and the range of blocks we're allowed to query with each call? The receivable scanner only knows new_start_block because blockchain_bridge sends it in the message. What happens if we receive no payments? Why is blockchain_bridge sending a message with empty transactions? Isn't that processing cost we don't need to incur? What if the receivable scanner is slow and it hasn't written the new_start_block before the blockchain_bridge fetches another set of transactions? It will use the old start_block_nbr again, correct? Do we know if this is an impossible scenario?

I have an answer for ReceivedPayments.new_start_block: Remove it like it was before November 2023.
If we have set_start_block in its rightful place in blockchain_bridge we don't always have to send the ReceivedPayments message to the Accountant. Only when there are transactions to scan. If we don't send new_start_block at all that totally cleans up trying to set_start_block in finish_scan and handle_new_received_payments. Let the Accountant worry about ReceivedPayments and let the BlockchainBridge worry about the start_block.

The reason it's Option<u64> is because PersistentConfiguration declares:

fn start_block(&self) -> Result<Option<u64>, PersistentConfigError>; fn set_start_block(&mut self, value_opt: Option<u64>) -> Result<(), PersistentConfigError>;

making this call very simple:

self.persistent_configuration.set_start_block(msg.new_start_block)

Ok. If I could strike through the previous comments I would, but GitHub markdown doesn't support it. This has percolated in my subconscious and I realize I am wrong and you are right. All you had to mention is that we have to commit the new_start_block in the same transaction with the ReceivedPayments. I will also make the ReceivedPayments message have new_start_block: u64 no Option needed.

bertllll · 2024-09-24T22:22:49Z

node/src/accountant/scanners/mod.rs

+                match txn.commit() {
+                    Ok(_) => debug!(logger, "The new_start_block ({}) is not greater than the current_start_block ({}). \
+                    Committed received transactions.", &new_start_block, &current_start_block),
+                    Err(e) => panic!("Commit of received transactions failed: {:?}", e),


Another and even less cool thing I'm witnessing is that I can't see any new tests that support this new code. There should be at least one test modified for the happy path (as I expect there might've been some sort of happy path before too), or a fully-new test, if somebody did a floppy job. Plus, there definitely should be one brand new test that concentrates on the situation when the committing results in an error and so a panic.

These things are a must.

Please make this right.
You'll need to use this special version of a database transaction: TransactionSafeWrapper. Make it by using TransactionSafeWrapper::new_with_builder. Either get some inspiration from tests that have it in use or just let the code navigate you through since the builder is designed to be the only way of how you can construct a mock version which you can control. And you do want to.

You need to assert by commit_params that this function was called, that for the happy path. In the sad path, you are just obligated to ensure the mock will return an error during that function call. So choose an error to be spit out, by setting it up with commit_result.

You're still possessing quite a lot of work to do. Please keep yourself better-focused this time.

I mentioned this in the call on Sunday. Why don't I already find TransactionSafeWrapper in use in accountant/scanners/mod.rs I didn't introduce txn.commit() for the first time.

I tried to trace even the happy path using the added logging but I didn't find it.

If you haven't seen my comment just before this one, please read.

I will provide adequate coverage for these changes and hopefully eliminate the extra paths. so it will be as it was before GH-606 in this part of the code except for passing Some(msg.new_start_block) to be persisted.

I do still think there is benefit for not sending the ReceivedPayements message when the blockchain transactions are empty. In that case I think it would be more efficient to have blockchain_bridge store the new_start_block but only if there are no transactions to send. But if you feel it's bad design to store the value in 2 places even though it's protected from conflict by the aformentioned condition and the message sending overhead is not too costly, I'll implement it how you decide. Please tell me.

masqrauder · 2024-09-30T23:31:41Z

node/src/blockchain/blockchain_bridge.rs

+        // let after = SystemTime::now();
+        let set_start_block_params = set_start_block_params_arc.lock().unwrap();
+        assert_eq!(*set_start_block_params, vec![Some(7)]);
+        // let accountant_received_payment = accountant_recording_arc.lock().unwrap();
+        // let received_payments = accountant_received_payment.get_record::<ReceivedPayments>(0);
+        // check_timestamp(before, received_payments.timestamp, after);
+        // assert_eq!(
+        //     received_payments,
+        //     &ReceivedPayments {
+        //         timestamp: received_payments.timestamp,
+        //         payments: vec![],
+        //         new_start_block: 7,
+        //         response_skeleton_opt: Some(ResponseSkeleton {
+        //             client_id: 1234,
+        //             context_id: 4321
+        //         }),
+        //     }
+        // );
+        let test_log_handler = TestLogHandler::new();
+        test_log_handler.exists_log_containing("DEBUG: BlockchainBridge: Write new start block: 7");
+        test_log_handler


@bertllll Depending on your response, I will either uncomment the code in this test to make sure the ReceivedPayments message is always sent even if the transactions are empty so the start_block_nbr is updated in accountant/scanner/mod. The alternative is to not send the message when there are no transactions and update the start_block_nbr directly in blockchain_bridge and save the message sending overhead.

masqrauder · 2024-09-30T23:36:05Z

node/src/blockchain/blockchain_bridge.rs

+        // let accountant_received_payment = accountant_recording_arc.lock().unwrap();
+        // let received_payments = accountant_received_payment.get_record::<ReceivedPayments>(0);
+        // check_timestamp(before, received_payments.timestamp, after);
+        // assert_eq!(
+        //     received_payments,
+        //     &ReceivedPayments {
+        //         timestamp: received_payments.timestamp,
+        //         payments: vec![],
+        //         new_start_block: 7,
+        //         response_skeleton_opt: Some(ResponseSkeleton {
+        //             client_id: 1234,
+        //             context_id: 4321
+        //         }),
+        //     }
+        // );


I was expecting to see my latest comment here on this commit but it doesn't appear because I made it from the main PR files view.

bertllll

I'll be happy to approve your changes when you have had a look at these minor issues and tried to work them out.

bertllll · 2024-10-15T23:57:41Z

node/src/blockchain/blockchain_bridge.rs

@@ -1639,13 +1627,10 @@ mod tests {
                transactions: vec![],


This is not related to the current card, but I noticed that the line 1623 (GitHub doesn't let me leave the comment there in place) is silly. It says that when we query for knowing what number is of the head block at the moment we receive a response saying it's just 0. The problem is it never can be so if we had already saved in the database the number 6 for the next position to start from.

Then I realised the test doesn't care because the whole blockchain interface is mocked and that questioned method is only called in the real version.

The bottom line is, that you probably should get rid of the whole entry for lower_interface. That means also the lower_interface_results are something what can get gone without any impact.

bertllll · 2024-10-16T00:01:25Z

multinode_integration_tests/docker/blockchain/entrypoint.sh

+    -p 18545 \
+    --networkId 2 \
+    --verbose \
+    --mnemonic "timber cage wide hawk phone shaft pattern movie army dizzy hen tackle lamp absent write kind term toddler sphere ripple idle dragon curious hold"


No probs. I'm no longer concerned about this. You can leave it in if you want to.

bertllll · 2024-10-16T00:24:42Z

node/src/blockchain/blockchain_interface/blockchain_interface_web3/mod.rs

+    fn blockchain_interface_retrieve_transactions_start_and_end_blocks_can_be_latest() {
+        let port = find_free_port();
+        let _test_server = TestServer::start (port, vec![
+            br#"[{"jsonrpc":"2.0","id":1,"result":"error"},{"jsonrpc":"2.0","id":2,"result":[{"address":"0xcd6c588e005032dd882cd43bf53a32129be81302","blockHash":"0x1a24b9169cbaec3f6effa1f600b70c7ab9e8e86db44062b49132a4415d26732a","data":"0x0000000000000000000000000000000000000000000000000010000000000000","logIndex":"0x0","removed":false,"topics":["0xddf252ad1be2c89b69c2b068fc378daa952ba7f163c4a11628f55a4df523b3ef","0x0000000000000000000000003f69f9efd4f2592fd70be8c32ecd9dce71c472fc","0x000000000000000000000000adc1853c7859369639eb414b6342b36288fe6092"],"transactionHash":"0x955cec6ac4f832911ab894ce16aa22c3003f46deff3f7165b32700d2f5ff0681","transactionIndex":"0x0"}]}]"#.to_vec()


I just don't understand what drove you to make the first transaction in the batch return an error. What does it imply in relation to the test? If it's actual the crucial peace of data in the response which set the right path in the code to go along it, maybe we should make it more obvious to the potential reader knowing about the context even less than for example me.

Maybe you could do some gymnastics
like this:

let error_causing_to_pick_fallback_value = "error"; let _test_server = TestServer::start (port, vec![ concat!(br#"[{"jsonrpc":"2.0","id":1,"result":"#, error_causing_to_pick_fallback_value, br#"},{"jsonrpc":"2.0","id":2,"result":[{"address":"0xcd6c588e005032dd882cd43bf53a32129be81302" ..."#

bertllll · 2024-10-16T00:39:52Z

node/src/node_configurator/configurator.rs

-        let block_number = match string_number.parse::<u64>() {
-            Ok(num) => num,
-            Err(e) => return Err((NON_PARSABLE_VALUE, format!("start block: {:?}", e))),
+        let block_number_opt = if "none".eq_ignore_ascii_case(&string_number) {


I agree with your take on this but it isn't really addressing what I was poking into. I can go along with you that sometimes my opinions are extreme, as well as here: I just pointed out in my old comment that your only test doesn't cover the behaviour which was added by incorporating the eq_ignore_ascii_case. It really is extreme because my reasoning stem from "if somebody went and rewrote the place with some different code, yet accepting "none", there is no guarantee it would still be code supporting also the other letter-casing. Here it is slightly hilarious, but it raises the question whether it is important that this extra little functionality is in there.

Okay, restating myself: my concern was that the test cannot tell apart, if the production code uses

let block_number_opt = if "none".eq_ignore_ascii_case(&string_number)...

or simply

let block_number_opt = if string_number == "none" {...

You see what I mean?

- 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-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 <Bert@Bert.com> * update readme and tag v0.8.1 (#532) Signed-off-by: KauriHero <kaurihero@masq.ai> * 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 <kaurihero@masq.ai> Co-authored-by: MASQrauder <60554948+masqrauder@users.noreply.github.com> Co-authored-by: Bert <65427484+bertllll@users.noreply.github.com> Co-authored-by: Bert <Bert@Bert.com> Co-authored-by: KauriHero <kaurihero@masq.ai> Co-authored-by: Syther007 <null.0.node@protonmail.com>

masqrauder self-assigned this Jan 21, 2024

masqrauder requested review from dnwiebe, czarte, utkarshg6, bertllll and Syther007 January 21, 2024 21:01

masqrauder force-pushed the GH-606 branch from 893b10c to b7ed3de Compare January 22, 2024 01:37

bertllll reviewed Jan 23, 2024

View reviewed changes

masqrauder force-pushed the GH-606 branch from b7ed3de to b22b8e5 Compare March 16, 2024 02:18

masqrauder force-pushed the GH-606 branch from b22b8e5 to b0f9e02 Compare March 31, 2024 17:19

masqrauder force-pushed the GH-606 branch from 1f740b2 to 82f39d6 Compare April 21, 2024 19:02

masqrauder force-pushed the GH-606 branch from 9226df7 to e397bf3 Compare June 16, 2024 02:57

masqrauder force-pushed the GH-606 branch from 049fc0e to bda6f39 Compare July 11, 2024 01:35

masqrauder requested a review from bertllll September 24, 2024 00:01

masqrauder force-pushed the GH-606 branch from 0afe468 to 55f5437 Compare September 24, 2024 00:35

masqrauder commented Sep 24, 2024

View reviewed changes

masqrauder force-pushed the GH-606 branch 2 times, most recently from 3784786 to d93e62c Compare September 24, 2024 00:42

bertllll reviewed Sep 24, 2024

View reviewed changes

masqrauder commented Sep 30, 2024

View reviewed changes

masqrauder requested a review from bertllll September 30, 2024 23:32

masqrauder commented Sep 30, 2024

View reviewed changes

masqrauder force-pushed the GH-606 branch 2 times, most recently from f6a59f0 to 0a957aa Compare October 3, 2024 05:28

masqrauder force-pushed the GH-606 branch from 0a957aa to 59ee2b8 Compare October 10, 2024 03:51

bertllll reviewed Oct 16, 2024

View reviewed changes

masqrauder added 4 commits October 20, 2024 16:07

GH-606: Initialize start_block to none to use latest block

ed7ca69

GH-606: Apply PR feedback changes

c61be52

GH-606: Apply PR feedback changes

aa54ff0

GH-606: Apply PR feedback changes

2a0e285

masqrauder added 5 commits October 20, 2024 16:07

GH-606: Apply PR review 4 feedback changes

c965035

GH-606: Squashing commits

f7938de

- 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

89e7171

GH-606: PR Feedback - parameterize a test

382f0de

GH-606: Address PR feedback

7c9b69d

masqrauder force-pushed the GH-606 branch from 59ee2b8 to 7c9b69d Compare October 22, 2024 02:21

GH-606: Implement parameterized test without crate macro

a9c7faf

kauri-hero self-requested a review November 6, 2024 00:37

bertllll approved these changes Nov 6, 2024

View reviewed changes

masqrauder merged commit 0caf945 into master Nov 6, 2024
6 checks passed

This was referenced Nov 6, 2024

GH-606-Review-5 #451

Closed

GH-606-Review-4 #449

Closed

GH-606-Review-3 #436

Closed

GH-606: Review 2 #429

Closed

		@@ -126,6 +130,7 @@ mod tests {
		fn validate_start_block_works() {

		@@ -0,0 +1,71 @@
		use crate::database::db_migrations::db_migrator::DatabaseMigration;
		use crate::database::db_migrations::migrator_utils::DBMigDeclarator;

GH-606: Initialize start_block to none to use latest block #374

GH-606: Initialize start_block to none to use latest block #374

Conversation

masqrauder commented Jan 21, 2024

bertllll left a 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

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

masqrauder Sep 25, 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

masqrauder Sep 26, 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 left a 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

masqrauder Sep 25, 2024 •

edited

Loading

masqrauder Sep 26, 2024 •

edited

Loading