Merge development into feature-dan #4687
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Description --- Updates tari_crypto to v0.15.5
Description --- Fixed small latent cucumber errors Motivation and Context --- Some cucumber tests were failing How Has This Been Tested? --- - `npm test -- --profile "none" --name "Wallet sending and receiving one-sided stealth transactions"` - `npm test -- --profile "none" --name "Node rolls back reorg on invalid block"`
tari-project#4564) Description --- Removed spawn blocking calls for db operations from the wallet in the key manager service. (This is another PR in a couple of PRs required to implement this fully throughout the wallet code.) Motivation and Context --- As per tari-project#3982 and tari-project#4555 How Has This Been Tested? --- Unit tests Cucumber tests
Description --- tari-project#4404 Motivation and Context --- console_wallet does not recognize password config setting, runtime error & exists tari-project#4404 How Has This Been Tested? --- manually (kudos to Stan)
Description --- Updates tokio to 1.20 Motivation and Context --- Includes many bug fixes: https://github.com/tokio-rs/tokio/blob/master/tokio/CHANGELOG.md How Has This Been Tested? --- Existing tests, manually (base node, wallet, miner)
… (tari-project#4570) Description --- - removes NodeId destination variant from DHT protocol - use public key destination for join message - use public key destination for wallet-to-wallet transaction messages Motivation and Context --- Ref tari-project#4139 - because any node id can be used the recipient is not necessarily bound in the signature. NodeId destination was not particularly useful and not required for messaging. It was used for DHT join messages and wallet-to-wallet transaction messages. How Has This Been Tested? --- Existing tests updated and pass Memorynet passes Cucumber tests pass Manually, sending a transaction via SAF
Description --- Adds a test to check ban login in DHT. The test creates a `NodeId` with invalid signature and tries to offer it as a peer. Motivation and Context --- To cover ban logic with tests. How Has This Been Tested? --- CI Co-authored-by: Stan Bondi <[email protected]>
…ect#4571) Description --- We address both (ignored) tests `fn version()` and `fn sender_offset_public_key` for consensus encoding, Motivation and Context --- The `TransactionInput` canonical hash and `TransactionOutput` hash currently miss specific fields, namely `version` and `spender_offset_public_key`. We address this following [RFC](https://rfc.tari.com/RFC-0121_ConsensusEncoding.html#transaction-input), and resolve the tests mentioned above. How Has This Been Tested? --- Unit tests
Update tor seeds for esmeralda
…ct#4563) Description --- The wallet uses two services, the OMS and TMS. The coinbase handling only happens inside of the TMS. When the TMS validates the coinbase and decides that the coinbase has been abandoned, it will update the TMS and then update the OMS. If the OMS update fails, then the TMS will have been updated and the OMS not, causing the two databases to be out of sync. And because the logic that determines if a coinbase has to be updated lives with the TMS, it will never now it still needs to update the OMS. Causing the OMS to always have pending_incoming output. This PR updates the flow to first update the OMS so when this fails, it can at a later date try to update the TMS. If the TMS fails, it will update the TMS to make sure its correct. Fixes: tari-project#4505
…ect#4567) Description --- Delete Seed words commands from GRPC tari-project#4363 tari-project#4363 Motivation and Context --- We need to remove this from GRPC service, this is a dangerous call. How Has This Been Tested? --- existing unit tests
…ari-project#4575) Description --- - Removed spawn blocking calls for db operations from the wallet in the contacts service. (This is another PR in a couple of PRs required to implement this fully throughout the wallet code.) - Reset the wallet's default db connection pool size back to 16 (from 5). Motivation and Context --- As per tari-project#3982 and tari-project#4555 How Has This Been Tested? --- Unit tests Cucumber tests System-level test
…-project#4561) (tari-project#4577) Description --- Resolve ignored tests in output manager service. Motivation and Context --- The given tests are failing mainly due to incorrectly hardcoded values. We address these issues. How Has This Been Tested? --- Unit tests
Description --- - updates remaining crates to 1.20 Motivation and Context --- Some crates used 1.10 and others 1.14 - search and replace missed 1.14 How Has This Been Tested? --- Search for all tokio deps and make sure they are set to 1.20
…reveal the address tari-project#4403 (tari-project#4516) Description --- tari-project#4403 Wallet (console and FFI) should have setting to not choose outputs that reveal the address tari-project#4403 Motivation and Context --- Problem Wallets currently will choose the best outputs as inputs when spending, however since a lurking base node can generate a transaction graph of inputs to outputs with relative ease, a wallet may reveal its transaction history by including a (non-stealth address) one-sided payment. For example, an attacker wishing to know the transaction graph of a public key PK_Alice can send a one-sided payment to PK_Alice using the Tariscript Push(PK_Alice). At some point, Alice's wallet spends this transaction without realizing it. Possible solution Could change the wallet to have a config setting, to not include one-sided payments by default when spending How Has This Been Tested? ---
Description --- A race condition exists if more than one miner asks for a coinbase with different values. The first transaction will be canceled by the second one. Coinbase transactions should only ever be canceled by the validation process after confirming they have not been mined.
…ject#4580) Description --- Attempts to recognize the source of a recovered output Motivation and Context --- Recovered outputs lack details of how they are received, so this is an attempt to perform at least some recognition. How Has This Been Tested? --- existing unit tests
…i-project#4591) Description --- Removed spawn blocking calls for db operations from the wallet in the wallet storage. (This is another PR in a couple of PRs required to implement this fully throughout the wallet code.) Motivation and Context --- As per tari-project#3982 and tari-project#4555 How Has This Been Tested? --- Unit tests Cucumber tests
Description
---
Fixed latent transaction service unit test errors
Motivation and Context
---
Some failing tests due to recent coinbase handling logic changes:
```
failures:
---- transaction_service_tests::service::test_coinbase_transaction_reused_for_same_height stdout ----
thread 'transaction_service_tests::service::test_coinbase_transaction_reused_for_same_height' panicked at 'assertion failed: `(left == right)`
left: `2`,
right: `1`', base_layer/wallet/tests/transaction_service_tests/service.rs:3968:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- transaction_service_tests::service::test_coinbase_generation_and_monitoring stdout ----
thread 'transaction_service_tests::service::test_coinbase_generation_and_monitoring' panicked at 'assertion failed: `(left == right)`
left: `Coinbase`,
right: `MinedUnconfirmed`', base_layer/wallet/tests/transaction_service_tests/service.rs:3405:5
---- transaction_service_tests::service::test_transaction_resending stdout ----
thread 'transaction_service_tests::service::test_transaction_resending' panicked at 'assertion failed: alice_ts_interface.outbound_service_mock_state.wait_call_count(1,\n Duration::from_secs(5)).await.is_err()', base_layer/wallet/tests/transaction_service_tests/service.rs:4181:5
failures:
transaction_service_tests::service::test_coinbase_generation_and_monitoring
transaction_service_tests::service::test_coinbase_transaction_reused_for_same_height
transaction_service_tests::service::test_transaction_resending
test result: FAILED. 39 passed; 3 failed; 0 ignored; 0 measured; 0 filtered out; finished in 107.87s
```
How Has This Been Tested?
---
Failing uni tests passed
Description --- Both wallets submit transactions. This means that for every tx created at least 1 warn!(target: LOG_TARGET, "Validation failed due to unknown inputs" will be created. This reduces the log level for this so that it does not popup by default.
…oject#4587) (tari-project#4592) Description --- It is desirable that the Merge mining proxy has a GRPC authenticated wallet client connection. Motivation and Context --- Contrary to the Mining wallet client connection, the merge mining proxy does not have GRPC authentication currently. This issue aims to add it to GRPC auth in the merge mining proxy. Fixes tari-project#4587. How Has This Been Tested? --- Existing unit tests
Description --- - Tests made to pass by marking the culprit test as `@broken` - Root cause is not fixed yet Motivation and Context --- FFI cucumber tests are not passing. How Has This Been Tested? --- This works now: `npm test -- --profile "none" --tags "@critical and not @long-running and not @broken and @wallet-ffi"`
Description --- This PR improves how the wallet displays and handles recovered coinbases. Motivation and Context --- Most of the coinbase transactions, have all of the required information to properly import then with the correct info on the utxo, so we don't have to flag them as imported. How Has This Been Tested? --- Manual Fixes: tari-project#4581
…ing (transaction service) (tari-project#4597) Description --- Removed spawn blocking calls for db operations from the wallet in the transaction service. (This is the last PR in a couple of PRs required to implement this fully throughout the wallet code.) Motivation and Context --- As per tari-project#3982 and tari-project#4555 How Has This Been Tested? --- Unit tests Cucumber tests
remove terminal resize on start up
Description --- - updates `yamux` from `0.9.0` to `0.10.2` - updates `snow` from `0.8.0` to `0.9.0` Motivation and Context --- [Yamux changelog](https://github.com/libp2p/rust-yamux/blob/master/CHANGELOG.md) [Snow changes](https://github.com/mcginty/snow/commits/master) How Has This Been Tested? --- Manually: Joined existing esme network, with wallet and base nodes and they continued to work
…ari-project#4598) Description --- - use RPC pool in wallet connectivity when scanning for UTXOs from the base node peer - extra: make potentially long-running `ping-peer` async Motivation and Context --- Fixes the wallet exceeding the max RPC sessions per peer limit of 10. ``` 09:53 WARN Rejecting handshake because no more RPC sessions available 09:53 ERROR error=Maximum number of client RPC sessions reached for node ee4baee242d0baffcab6ef5f20 ``` How Has This Been Tested? --- Manual, 6 wallets connected to one base node, UTXO scanning and recovery and a number of stress tests - max rpc sessions were used but not exceeded
…ari-project#4607) Description --- - only release connection handles of non-neighbouring peers after successful connect - adds min threshold for connection reaping with default 50 - only reap connections that have less than 3 substreams - only reap "excess" (num_connections - 50) connections - adds RpcServer query that returns number of sessions for a peer - updates list-connections command to display number of peer connection handles and rpc sessions - updates list-connections to display two tables, one with wallets and one with base nodes Motivation and Context --- Previously, connection handles would be dropped (making them reapable) when refreshing the neighbour peer pool. The neighbour pool only starts the attempt to connect, but the non-neighbouring connection handles should only be dropped if a replacement neighbour was actually able to connect. Reaping should only apply when we have many connections, otherwise many connections are acceptable. Fixes tari-project#4608 How Has This Been Tested? --- Manually, checking that connections to other base nodes/wallets running this PR stay connected
Description Switch out ubuntu dependencies for script from workflow Motivation and Context Using a single bash script makes updates easier. Also local installs can use the same ubuntu dependencies install script. How Has This Been Tested? Only clippy has been tested
Description --- - removes unused dependencies found by `cargo udeps` - remove `tari_common_types` dep from tari_script
…ect#4627) Description --- Changes the order of validation in mempool to check the excess signature before checking the inputs. Motivation and Context --- When receiving old transactions, if we check the inputs first, the tx will fail on input validation, and we flag this as a transaction that double spends some input. This is incorrect as it's a rebroadcast of an old already mined transaction. So we check the kernel_excess signature first, and if this is already contained in the blockchain, we just print out a debug message saying we received an old already mined kernel.
…#4633) Description --- Change log level from warn to trace Motivation and Context --- Currently its logging like this when pickup up an old transaction: ``` // base_layer/core/src/transactions/aggregated_body.rs:449 2022-09-07 12:47:12.330454000 [c::val::transaction_validators] [cc1ade71a24700ddefc86457dbb6e1446229641b0e1c56bce909bcd0f1a0f950,7acd212bf3c3b5a0428ebfec90] WARN Block contains kernel excess: 7e466195f3ba58f88683b1da3fc628ec418b41b2f2b1f93e22afbc8fd4921b73 which matches already existing excess signature in chain database block hash: 38bd8d1a24b2512a56bacd8683843c167791767c97e9b294cc89714ce4b2f1d6. Existing kernel excess: 7e466195f3ba58f88683b1da3fc628ec418b41b2f2b1f93e22afbc8fd4921b73, excess sig nonce: ca1843aeab73ad01a3fa0f463ba916e77966fc223e85779ec7851f7b90c7a85d, excess signature: b68a423c5ffd80639722b687662d7d33006edc7da07bcdca7598a3d6e408b70e // base_layer/core/src/validation/transaction_validators.rs:174 2022-09-07 12:47:12.330458000 [c::mp::mempool_storage] [cc1ade71a24700ddefc86457dbb6e1446229641b0e1c56bce909bcd0f1a0f950,7acd212bf3c3b5a0428ebfec90] DEBUG Validation failed due to already mined kernel: Block contains kernel excess: 7e466195f3ba58f88683b1da3fc628ec418b41b2f2b1f93e22afbc8fd4921b73 which matches already existing excess signature in chain database block hash: 38bd8d1a24b2512a56bacd8683843c167791767c97e9b294cc89714ce4b2f1d6. Existing kernel excess: 7e466195f3ba58f88683b1da3fc628ec418b41b2f2b1f93e22afbc8fd4921b73, excess sig nonce: ca1843aeab73ad01a3fa0f463ba916e77966fc223e85779ec7851f7b90c7a85d, excess signature: b68a423c5ffd80639722b687662d7d33006edc7da07bcdca7598a3d6e408b70e ``` This PR changes the first message to trace, as the info is already logged as debug, and this might not be a problem.
Description Audit workflow has a bad commit, which is now fixed Motivation and Context Get audit workflow running again How Has This Been Tested? Not been tested
Description --- - fix(rpc/server): detect stream interrupt when waiting for responses from domain service and when sending any data - fix(rpc/client): detect early response stream drop while reading responses - fix(rpc): correct value for number of sessions per peer - fix(base_node/sync): use faster/simpler mutex for sync session tracking - fix(base-node): set force_sync_peers conf from `base_node.force_sync_peers` - tests(comms/rpc): test for session handling when a response stream with large messages is interrupted - fix(p2p/liveness): pong event returns the latency of the last ping/pong Motivation and Context --- Whenever sync latency exceeds the max, the client interrupts the stream. The server rarely picks this up and since only one sync session is allowed, the client node can not resume syncing from the same node. fixes tari-project#4630 ref tari-project#4646 (fixes but duplicate config should be removed) fixes tari-project#4636 How Has This Been Tested? --- New perviously failing integration test, manually by syncing a new base node
Description --- Fix the log `Block contains kernel excess` This validator is used by the mempool for transactions and by the base_nodes for blocks
Description libtor-sys requires autoconf & automake tools to build Motivation and Context Improve builds on Ubuntu How Has This Been Tested? Tested in a clean Vagrant VM using ```scripts/install_ubuntu_dependencies.sh```
…ct#4649) Description --- If the RPC deadline is reached, immediately move on to the next sync peer. Motivation and Context --- Fixes tari-project#4648 How Has This Been Tested? --- Manually, header, pruned and block sync
Description --- This replaces the [generalized Luhn](https://en.wikipedia.org/wiki/Luhn_mod_N_algorithm) checksum algorithm for emoji ID encoding with [DammSum](https://github.com/cypherstack/dammsum), and refactors `EmojiId` to be cleaner and easier to understand. Fixes [issue 4638](tari-project#4638). Motivation and Context --- Emoji IDs are encoded with a checksum that is intended to detect simple errors; these include single character substitutions and single transpositions of adjacent characters. The generalized Luhn checksum algorithm cannot detect all transpositions, though it has the benefit of being a single character. While checksums of longer length and greater complexity can provide more comprehensive error detection, this comes at the cost of a longer overall emoji ID encoding and (often) additional structures like lookup tables. DammSum is a simple single-character checksum algorithm based on the [Damm algorithm](https://en.wikipedia.org/wiki/Damm_algorithm) that provably detects the two desired error types in their entirety. It requires no lookup tables and uses only a small number of bitwise operations. Other options based on Reed-Solomon or CRC designs may be considered as well as alternatives. How Has This Been Tested? --- Existing tests should pass. New randomized tests are added that comprehensively check failure modes and error detection.
…i-project#4657) Description --- - Fixes possible rare deadlock when broadcasting many messages due to internal channel - Reduce number of inbound and outbound pipeline workers - Greatly reduce buffer size between inbound messaging and the inbound pipeline to allow for substream backpressure - Adds "last resort" timeout in outbound pipeline Motivation and Context --- The outbound pipeline could deadlock when all pipeline workers are busy, and the outbound sink service is full, causing the pipeline to wait for both a free executor slot and a free slot to send on the channel How Has This Been Tested? --- Memorynet, Manually: wallet stress tests (2 x wallets, 2 x base nodes), checked SAF message exchange
Description --- Allows fee estimate to always return an estimated fee. Motivation and Context --- Currently, when calling fee estimate and you do not have enough funds, or funds pending, the fee estimate will error. We do not require such a high level of accuracy when calculating a fee estimate for an external call for an estimation. When the wallet does not have enough funds available, the API will return the fee estimate for default with 1 input and 1 kernel. How Has This Been Tested? --- unit test
…#4547) (tari-project#4655) Description --- Adds command to burn funds on Tari console wallet. Motivation and Context --- Tackle issue tari-project#4547. How Has This Been Tested? --- Proper unit tests.
Description --- This fixes a deadlock in the code. The `RwLock` allows a mutex to exist that allows concurrent reads but blocks concurrent writes, this behaves differently depending on the OS. On Linux systems: The lock favors reads, meaning that as long as there exists a reader lock, it will allow a new reader lock to open. This means that the system can cause starvation of writers. On Mac/Win the lock has equal ordering, this means that as soon as a writer queues for a lock, all additional readers will be blocked till after the writer has acquired and released its lock. This behavior can be dangerous if recursive locks are used, as was the case here. At about the same time, a block was submitted, and a template was constructed for a new miner. The `add_block` process requires a write lock, while the `block template` process requires a read lock. The `template process` was first in acquiring a lock on the read, followed shortly by the `add_block` on the blocking for a write. But the deadlock was caused after the `add_block` blocked for a write, the `block template` required an additional read_lock on the calculation of the mmr roots. And thus, the entire `block_chain db `class is deadlocked. Fixes: tari-project#4668
…oject#4677) Description --- This fixes a potential race condition. It is possible for `add_block` to pass the `is_add_block_disabled()`, start doing orphan validation (which can take quite long). While this is happening, `sync` sets the `add_block_disabled` flag and acquires a read_lock to then do pre-processing to determine sync mode, etc. While this is busy, `add_block` asks for a write_lock. `Add_block` gets its write_lock before `sync` gets its write_block because of the prioritization of RWLock. Also moved the `if db.contains(&DbKey::BlockHash(block_hash))` before the orphan validation as this is a much cheaper operation.
Description --- Removes an old defunct, broken test. This test was used to test that the genesis block passed validation. We later changed the gen block to not be able to pass validation, thus adding the ValidatingGenesis error that was tested here. Genesis block now has its own test `esmeralda_genesis_sanity_check` that checks the sanity of the gen block. Add block now also checks if the block has already been added to the blockchain before doing any validation. These changes completely remove the need for this test. How Has This Been Tested? --- unit tests
…roject#4672) Description Replicate CircleCI tests for FFI in GHA Motivation and Context Use single CI workflow How Has This Been Tested? Run CI locally, but not able to get FFI tests to work, all other tests in local fork
…s and substream close (tari-project#4676) Description --- - Simplify outbound pipeline by removing the [pipeline] -> [messaging] channel - Pipe outbound messages directly to the messaging protocol instead of through the outbound pipeline - Fix rare lockup when calling yamux control close Motivation and Context --- The outbound pipeline needed to poll two channels in order to make progress, some code branches in the outbound pipeline may need to use the other channel, and if that channel is full and the number of concurrent outbound tasks are full, a deadlock will occur. This case has not been directly observed, but is technically possible so should be eliminated. This PR removes the [pipeline] -> [messaging] channel, making the outbound pipeline only have to poll one channel. It also directly pipes `OutboundMessage`s to the messaging protocol. EDIT: I believe I've found the root cause. The connectivity manager would rarely "lock up" causing the pipelines to lock up (both pipelines require calls to connectivity manager). I traced this in the logs and found that the last thing the connectivity manager does is resolve a tie break before locking up. This involves disconnecting one of the peer connections, and it appeared this future, extremely rarely, did not resolve. Digging deeper from there, I was able to track down a flaw in the substream close procedure, write a test that reproduces it and make a fix. How Has This Been Tested? --- Number of ~1000-2000tx stress tests, leaving base nodes overnight (none of these are conclusive but no issues were encountered)
Description --- Remove DETACH flag from tor hs initialization. Motivation and Context --- DETACH was previously added as an attempt to improve tor connection reliability, but allowing tor to keep the hidden service registered even after the node has shut down. However, this was never really confirmed to improve reliability. Since this causes tor to fail an assertion, and since testing shows that connections still function as expected, it is removed in this PR. How Has This Been Tested? --- Stopping and starting nodes and checking the tor log does not throw an error
Ran cargo update to update dependancies
…lled (tari-project#4686) Description --- - Sets output `mined_height` in addition to `mined_in_block` to NULL when cancelling pending transactions - renames `set_output_to_unmined` to `set_output_to_unmined_and_invalid` Motivation and Context --- This can cause tari-project#4670 and was introduced in tari-project#3863 `set_output_to_unmined` also marks the output as invalid which is unexpected given the name. How Has This Been Tested? --- Additional basic test for `set_output_to_unmined`
Description --- Stray clippy error fix from tari-project#4682
Description --- After removing transactions from the mempool from a failed validation, reinsert them to keep the valid ones Motivation and Context --- Currently, the implementation discards all transactions when validation fails. This is a pretty heavy approach, because the block may be incorrect in the header. There may even be an attack where a malicious user crafts a bad block and removes all transactions in the mempool. In this approach, the transactions are reinserted into the mempool. If any of them are now invalid (e.g. double spends) they will be discarded How Has This Been Tested? --- existing tests, CI
* development: (72 commits) fix: reinsert transactions from failed block (tari-project#4675) fix: stray clippy error (tari-project#4685) fix(wallet): mark mined_height as null when pending outputs are cancelled (tari-project#4686) chore: updated dependancies (tari-project#4684) fix(p2p): remove DETACH flag usage (tari-project#4682) fix(comms): simplify and remove possibility of deadlock from pipelines and substream close (tari-project#4676) feat(ci): add default CI and FFI testing with custom dispatch (tari-project#4672) chore: remove broken test (tari-project#4678) fix: fix potential race condition between add_block and sync (tari-project#4677) fix deadlock (tari-project#4674) fix: add burn funds command to console wallet (see issue tari-project#4547) (tari-project#4655) v0.38.3 fix: fee estimate (tari-project#4656) fix(comms/messaging): fix possible deadlock in outbound pipeline (tari-project#4657) fix: replace Luhn checksum with DammSum (tari-project#4639) fix(core/sync): handle deadline timeouts by changing peer (tari-project#4649) fix(ci): libtor build on Ubuntu (tari-project#4644) chore: fix log (tari-project#4634) v0.38.2 fix(comms/rpc): detect early close in all cases (tari-project#4647) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.