GH-606-Review-4 #449
GH-606-Review-4 #449
Conversation
* fixed tests failing on Bert's computer * formatting * remove commented out code * change .clone() to reference * fixed test for windows in actions: tilde_in_config_file_path_from_commandline_and_args_uploaded_from_config_file * target_os for dirs crate used only on windows test * implementing Bert's review comments - change DirsWrapperMock to DirsWrapperReal and simplifying home_dir creation * fixing windows data-directory assertion in test tilde_in_config_file_path_from_commandline_and_args_uploaded_from_config_file * fixing home dir assertion for windows * add condition to use of dirs crate only for windows * fixing config toml for windows test tilde_in_config_file_path_from_commandline_and_args_uploaded_from_config_file * fixed windows test * formatting * fixed tilde tests for windows * remove unnecesary function for finding project root, use of current dir for DirsWrapperMock * adjusting name of test and removing commented out code
* GH-791: fixed * GH-791: put some explanatory lines back --------- Co-authored-by: Bert <Bert@Bert.com>
* GH-797: minor docs modification * GH-797: some TODOs * GH-797: add the definitions for min hops and gas price. * GH-797: remove the markdown code container
* GH-728: add the TODOs * GH-728: removed the code of the NewPasswordMessage * GH-728: use ConfigurationChangeMessage while changing password * GH-728: rename configuration_change_msg_sub_opt to update_min_hops_sub_opt * GH-728: rename new_password_subs to update_password_subs * GH-728: fix wrong name for the actor message * GH-728: send ConfigurationChangeMessage to other actors too * GH-728: add a TODO * GH-728: rename to _opt * GH-728: implement handler for ConfigurationChange::UpdatePassword inside Neighborhood * GH-728: add TODO to implement handler for ConfigurationChange::UpdatePassword inside BlockchainBridge * GH-728: add TODO to implement handler for ConfigurationChange::UpdatePassword inside Accountant * GH-728: create new struct for UpdatePasswordSubs * GH-728: add the new TODO * GH-728: send a ConfigurtionChangeMessage when the consuming wallet is generated * GH-728: add a test for the panic case * GH-728: some refactor changes in configurator.rs * GH-728: test drive the case when the wallet is recovered * GH-728: fix some of the tests * GH-728: fix the remaining 2 failing tests * GH-728: reorder fields * GH-728: the received ConfigurationChangeMessage is properly handled * GH-728: update the password when necessary * GH-728: add a todo * GH-728: add the ability to synchronise password * GH-728: add ability for accountant to handle an unexpected configuration change msg * GH-728: improve tests for handling ConfigurationChangeMessage in accountant * GH-728: minor renames * GH-728: remove finished TODO and formatting changes * GH-728: add test in blockchain bridge for ConfigurationChangeMessage * GH-728: add test in neighborhood for ConfigurationChangeMessage * GH-728: remove useless tests * GH-728: revive an old test * GH-728: improve todo * GH-728: test rename and add more TODOs * GH-728: cleanup and creation of new file configuration_change_subs.rs * GH-728: minor refactor and remove tests * GH-728: introduce trait for ConfigurationChangesubs * GH-728: refactor out the subs for UpdateWallets and UpdatePassword * GH-728: some renames * GH-728: add TODO and minor reordering * GH-728: make the test the_password_is_synchronised_among_other_actors_when_modified stronger * GH-728: add formatting changes * GH-728: make enum usage more explicit * GH-728: remove clippy warnings * GH-728: do some renames * GH-728: fix wrong rename and import * GH-728: use peer actors for generating config change subs * GH-728: recreate ConfigChangeSubs * GH-728: remove config_change_subs.rs * GH-728: test for ConfigChangeMsg in Accountant contain assertions * GH-728: test for ConfigChangeMsg in Neighborhood contain assertions * GH-728: test for ConfigChangeMsg in BlockchainBridge contain assertions * GH-728: small rename * GH-728: add context_id to fn * GH-728: refactor the fn to transform associated functions to methods * GH-728: minor fixes * GH-728: add wallet_opt as a new argument in the begin_scan fn * GH-728: remove the use of field consuming_wallet_opt from BlockchainBridge * GH-728: remove the field consuming_wallet_opt from BlockchainBridge * GH-728: don't send the ConfigChangeMsg to the BlockchainBridge * GH-728: use the earning wallet in Receivable Scanner's begin_scan * GH-728: don't save a reference of earning wallet inside Scanners * GH-728: store the earning wallet directly without an Rc * GH-728: fix the failing test * GH-728: formatting changes * GH-728: remove the earning wallet from ReceivableScannerBuilder * GH-728: check wallet in the begin_scan() for ScannerMock * GH-728: remove unnecessary TODO * GH-728: formatting changes * GH-728: fix the failing test * GH-728: remove unused import in blockchain_interaction_test.rs * GH-728: BlockchainBridge receives consuming_wallet instead of an Option<T> * GH-728: Check consuming wallet before sending a msg to BlockchainBridge * GH-728: remove Option<T> from the wallet_opt param in begin_scan fn * GH-728: remove commented out code * GH-728: modify AccountantBuilder to make it's field names contain opt * GH-728: improve test mocks * GH-728: introduce another URL for mumbai testnet * GH-728: change the message in trace log * GH-728: test drive the NoConsumingWallet Error * GH-728: reordering * GH-728: add better logging * GH-728: Review 2 changes * GH-728: Review 2 leftover changes * GH-728: add better assertions for logger
| ); | ||
| if let Err(e) = self | ||
| .persistent_config | ||
| .set_start_block(Some(new_start_block_number.as_u64())) |
There was a problem hiding this comment.
This must be some kind of a merge issue. Now we should always set the start block in Accountant (Why? Because it must be done after all the retrieved transactions are written into the books. Otherwise we're risking impaired state of the books if we restart the Node following a panic or the OS's death, with the start block already advanced for the next scan but having lost some payment information, so with missing inaccurate figures in the database)
I'm sorry but it takes possibly all the code in this file, even the tests. What is useful, though, is the code you wrote in the Accountant. There it is okay.
There was a problem hiding this comment.
I don't think this is a merge issue. For example, if blockchain_bridge scans 10,000 blocks from 100 to 10,100 and the accountant processes payments received at blocks 4242, and 5758; blockchain_bridge may have persisted that it found all the incoming payments between 100 and 10,100 making it ready to start at 10,101 for the next scan. However, previously if accountant blindly records 4242 then 5758, blockchain_bridge might rescan blocks. By checking to avoid storing a smaller block number, accountant lowers the risk of repeat scans.
I'm reluctant to make such a big change in my card since it's gone on far too long already. I agree with you that it does make sense to record the block number according to what the accountant has successfully recorded. I still think there are some initialization times where we should set and advance the starting block number here in blockchain_bridge while we await our first receivable. If we start and stop the node without receiving any payments the start block number won't advance. When we do restart it will rescan the blocks since the last received payment.
I still argue what the accountant records should be idempotent. It should be safe to reprocess the same transaction block and it can be if we record the sending address, the block (or tx hash), and the amount, we'll never accidentally double count anything. It also would not be harmful to re-scan since it might occur despite our best efforts to prevent it. A database confiscated in a raid won't have any additional information that what is already on the public blockchain -- I agree we are providing a tidy collection of all the 'incriminating evidence' without all the noise. However, the overwhelming benefit is that the books are far less likely to become imbalanced. With the current implementation I suspect frequently declaring bankruptcy will be common.
node/src/blockchain/blockchain_interface/blockchain_interface_web3/mod.rs
Outdated
Show resolved
Hide resolved
| @@ -137,12 +137,12 @@ where | |||
| let response_block_number = match block_request.wait() { | |||
There was a problem hiding this comment.
Could you please use the suffix _opt for this variable and any similarly created? As now the typing is different, and it is actually optional.
There was a problem hiding this comment.
On the other hand, I haven't come across a line where this variable could resolve as None. It seems to me it can never be None. If you agree with me, you may want to simplify it, it would also help with the arguments in find_transactions_largest_block_number(). Which I'd also appreciate.
node/src/database/config_dumper.rs
Outdated
| value => assert!(value.is_null()), | ||
| } | ||
| .unwrap_or_else(|| panic!("record for {} is missing", key)); | ||
| assert!(value.is_null()) |
There was a problem hiding this comment.
Maybe you could provide a message to this macro which prints if it fails that would print the presented value if it was NOT null. It can make fixing failing tests easier.
| Ok(self.dao.set_by_guest_transaction( | ||
| txn, | ||
| parameter_name, | ||
| value.map(|v| v.to_string()).or(None), |
There was a problem hiding this comment.
You don't have to say
.or(None)if it is Option and you already provided what happens if it is Some. Only None can be the other value.
There was a problem hiding this comment.
Oh right. I'm surprised Rust or RustRover didn't make that recommendation.
Summary (from AI Assistant)
Changes
Adjusted the field new_start_block in struct ReceivedPayments to be of type
Option<u64>.Altered multiple unit tests to reflect the new field type.
Enforced an additional condition to only update the
new_start_blockif its value is greater than the current start block maintained in the persistent configuration.Similar to accountant::mod.rs, an additional condition was added before updating the
new_start_block.new_start_blockis now checked in two methods to ensure it isn't updated with lower values which would lead to reprocessing of previously processed blocks.Various unit tests adjusted to incorporate the changes related to
new_start_block.Note: The commit involves improvements and code quality changes and does not appear to add any new features or remove existing ones.