Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: finalize n-of-m #4822

Merged
merged 20 commits into from
Oct 25, 2022
Merged

Conversation

SWvheerden
Copy link
Collaborator

Description

This is the finalized n-of-m push, that includes all commands to make this.
This removes one or two integrity checks from the wallet transaction builder.

How Has This Been Tested?

Manual

sdbondi and others added 20 commits October 6, 2022 16:44
Description
---
Removed stray unwrap in liveness service

Motivation and Context
---
Caused a base node to panic in stress test conditions.

```
thread 'tokio-runtime-worker' panicked at 'called `Result::unwrap()` on an `Err` value: DhtOutboundError(RequesterReplyChannelClosed)', base_layer\p2p\src\services\liveness\service.rs:164:71
```

How Has This Been Tested?
---
Tests pass
…de/serialization (tari-project#4791)

Description
---
- Uses tari script encoding (equivalent to consensus encoding) for `ExecutionStack` serde impl
- Rename as_bytes to to_bytes as per rust convention.
- adds migration to fix execution stack encoding in db

Motivation and Context
---
Resolves tari-project#4790 

How Has This Been Tested?
---
Added test to alert if breaking changes occur with serde serialization for execution stack.
Manual testing in progress
Description
---
Transaction service sql db queries must handle  `DieselError(DatabaseError(__Unknown, "database is locked"))`. This PR attempts
  to remove situations where that error may occur under highly busy async cirumstances, specifically:
- Combine find and update/write type queries into one.
- Add sql transactions around complex tasks.

_**Note:** Partial resolution for tari-project#4731._

Motivation and Context
---
See above.

How Has This Been Tested?
---
- Passed unit tests.
- Passed cucumber tests.
- ~~**TODO:**~~ System level tests under stress conditions.
Description
---

This moves the nonce to the front of the hashing order when hashing for the sha3 difficulty. 
This is done so that mining cannot cache part most the header and only load the nonce in. This forces the miner to hash the complete header each time the nonce chances. 

Motivation and Context
---

Fixes: tari-project#4767 

How Has This Been Tested?
---
Unit tests all pass.
Description
---
- Ignores nanos for `stored_at` field in StoredMessages
- Uses direct u32 <-> i32 conversion
- Improve error message if attempting to store an expired message
- Discard expired messages immediately
- Debug log when remote client closes the connection in RPC server

Motivation and Context
---
- Nano conversion will fail when >= 2_000_000_000, nanos are not important to preserve so we ignore them (set to zero)
- u32 to/from i32 conversion does not lose any data as both are 32-bit, only used as i32 in the database 
- 'The message was not valid for store and forward' occurs if the message has expired, this PR uses a more descriptive error message for this specific case.
- Expired messages should be discarded immediately
- Early close "errors" on the rpc server simply indicate that the client went away, which is expected and not something that the server controls, and so is logged at debug level 

How Has This Been Tested?
---
Manually,
Description
---
Adds conditional to only increase database size if migration is required

Motivation and Context
---
A new database (cucumber, functional tests) has no inputs and so migration is not required.
Ref tari-project#4791 

How Has This Been Tested?
---
Description
---
Removes unused function in miner

Motivation and Context
---
Clippy

How Has This Been Tested?
---
No clippy error
Description
---
* remove auto update tests from cucumber
* rename some tests to be prefixed with `test_`
* simplified two cucumber tests by removing steps

Motivation and Context
---
The auto update tests have an external dependency, which makes it hard to test reliably. They were marked as broken, so I rather removed them.
There were two steps in the `list_height` and `list_headers` tests that created base nodes. Upon inspection of the logs, these base nodes never synced to the height of 5 and were  not checked in the test, so were pretty useless and just slowed the test down 

How Has This Been Tested?
---
npm test
@CjS77 CjS77 added CR-too_long Changes Requested - Your PR is too long CR-insufficient_context Your PRs commit messages don't provide enough context to justify accepting the change. labels Oct 19, 2022
@CjS77 CjS77 added the P-acks_required Process - Requires more ACKs or utACKs label Oct 25, 2022
@stringhandler stringhandler merged commit b230af3 into tari-project:feature-m-of-n Oct 25, 2022
@SWvheerden SWvheerden deleted the sw_finm-n branch November 10, 2022 05:54
hansieodendaal added a commit to hansieodendaal/tari that referenced this pull request Jun 24, 2024
hansieodendaal added a commit to hansieodendaal/tari that referenced this pull request Jun 24, 2024
hansieodendaal added a commit to hansieodendaal/tari that referenced this pull request Jun 24, 2024
@hansieodendaal hansieodendaal mentioned this pull request Jun 24, 2024
4 tasks
hansieodendaal added a commit to hansieodendaal/tari that referenced this pull request Jun 24, 2024
hansieodendaal added a commit to hansieodendaal/tari that referenced this pull request Jun 24, 2024
hansieodendaal added a commit to hansieodendaal/tari that referenced this pull request Jun 26, 2024
hansieodendaal added a commit to hansieodendaal/tari that referenced this pull request Jun 27, 2024
SWvheerden pushed a commit that referenced this pull request Jun 28, 2024
Description
---

Apply m_of_n_feature branch

- PR #4751
- PRs #4759, #4776, #4798, #4822

Motivation and Context
---

Add m-of-n scripting ability

How Has This Been Tested?
---

**Note:** Not tested yet

What process can a PR reviewer use to test or verify this change?
---

Code review

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR-insufficient_context Your PRs commit messages don't provide enough context to justify accepting the change. CR-too_long Changes Requested - Your PR is too long P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants