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.

Sign up for GitHub

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

Jump to bottom

Remove GRPC via feature flag in wallet + 1-sided mining #5930

Closed
SWvheerden opened this issue Nov 9, 2023 · 5 comments
Closed

Remove GRPC via feature flag in wallet + 1-sided mining #5930

SWvheerden opened this issue Nov 9, 2023 · 5 comments
Assignees
@hansieodendaal
Labels
release-blocker Something that needs to be fixed before a release can be made

Comments

@SWvheerden
Copy link
Collaborator

SWvheerden commented Nov 9, 2023 •
edited
Loading

We should hide and remove GRPC from the wallet only adding it in via a compile feature flag.
The main reason for GRPC is the wallet is mining.
This requires we must change mining to be one-sided.
@SWvheerden SWvheerden added the release-blocker Something that needs to be fixed before a release can be made label Nov 9, 2023
@SWvheerden SWvheerden mentioned this issue Nov 9, 2023
Closed
@SWvheerden SWvheerden changed the title Remove GRPC via feature flag in wallet Remove GRPC via feature flag in wallet + 1-sided mining Nov 9, 2023
@hansieodendaal hansieodendaal self-assigned this Nov 9, 2023
@SWvheerden SWvheerden mentioned this issue Nov 17, 2023
Closed
@hansieodendaal hansieodendaal mentioned this issue Nov 18, 2023
Merged
4 tasks
@hansieodendaal
Copy link
Contributor

See #5967

SWvheerden pushed a commit that referenced this issue Nov 23, 2023
@hansieodendaal
feat!: add one-sided coinbase payments (#5967)
89b19f6 
Description
---
Added normal one-sided and stealth one-sided coinbase transactions.
Changes are:
- Coinbases can now only be paid to a nominated wallet address directly.
- The minotari miner and merge mining proxy will only start if a valid
minotari wallet address has been supplied for the particular network.
- The miner and merge mining proxy does not depend on the wallet anymore
to construct the coinbase; it is done directly.
- Corresponding gRPC method (`rpc GetCoinbase (GetCoinbaseRequest)
returns (GetCoinbaseResponse)`) has been removed from the wallet.
- A memory-based transactions key manager has been created for use by
the miner and merge mining proxy as they do not need persistent storage
of keys. This also ensures that new entropy for the generation of keys
is created each time the miner or merge mining proxy is started
effectively negating any collisions of private keys.
- All unused code in the output manager and transaction manager have
been removed.

Motivation and Context
---
See #5930

How Has This Been Tested?
---
Unit tests
Cucumber tests
System-level tests - _Completed_

What process can a PR reviewer use to test or verify this change?
---
- Code walk-through
- Run cucumber `Scenario: Get Transaction Info` and review the log files
for `WALLET_A` where the stealth one-sided coinbases will be imported
and where payment is made to `WALLET_B`.


<!-- 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
---

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

<!-- Does this include a breaking change? If so, include this line as a
footer -->
- Coinbases can now only be paid to a nominated wallet address directly.
- Existing wallets need to be recovered into new wallets.
@hansieodendaal
Copy link
Contributor

In the above PR the related gRPC methods and structs that supported mining have been removed from the wallet.

I do not think having a feature flag to hide the remaining gRPC methods is necessary, as we can implement grpc_server_deny_methods by default similar to what is done in the base node.

@CjS77
Copy link
Collaborator

CjS77 commented Nov 24, 2023

Note that launchpad uses gRPC to control the wallet via the docker API.

So the builds for the wallet docker container should be updated to include the gRPC feature when this change makes it into the code.

@SWvheerden
Copy link
Collaborator Author

We should still feature flag this out. And make sure the dockers compile with the feature flag.

The configs are not encrypted or protected in anyway, so enabling that is super easy

@SWvheerden SWvheerden mentioned this issue Nov 27, 2023
Closed
sdbondi pushed a commit to sdbondi/tari that referenced this issue Nov 27, 2023
@hansieodendaal @sdbondi
feat!: add one-sided coinbase payments (tari-project#5967)
6179995 
Description
---
Added normal one-sided and stealth one-sided coinbase transactions.
Changes are:
- Coinbases can now only be paid to a nominated wallet address directly.
- The minotari miner and merge mining proxy will only start if a valid
minotari wallet address has been supplied for the particular network.
- The miner and merge mining proxy does not depend on the wallet anymore
to construct the coinbase; it is done directly.
- Corresponding gRPC method (`rpc GetCoinbase (GetCoinbaseRequest)
returns (GetCoinbaseResponse)`) has been removed from the wallet.
- A memory-based transactions key manager has been created for use by
the miner and merge mining proxy as they do not need persistent storage
of keys. This also ensures that new entropy for the generation of keys
is created each time the miner or merge mining proxy is started
effectively negating any collisions of private keys.
- All unused code in the output manager and transaction manager have
been removed.

Motivation and Context
---
See tari-project#5930

How Has This Been Tested?
---
Unit tests
Cucumber tests
System-level tests - _Completed_

What process can a PR reviewer use to test or verify this change?
---
- Code walk-through
- Run cucumber `Scenario: Get Transaction Info` and review the log files
for `WALLET_A` where the stealth one-sided coinbases will be imported
and where payment is made to `WALLET_B`.


<!-- 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
---

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

<!-- Does this include a breaking change? If so, include this line as a
footer -->
- Coinbases can now only be paid to a nominated wallet address directly.
- Existing wallets need to be recovered into new wallets.
@hansieodendaal hansieodendaal mentioned this issue Nov 27, 2023
Merged
4 tasks
@hansieodendaal
Copy link
Contributor

See #5988 where the features = ["grpc"] feature flag is added.

SWvheerden pushed a commit that referenced this issue Nov 28, 2023
@hansieodendaal
feat!: disable console wallet grpc (#5988)
883de17 
Description
---
Disabled the console wallet gRPC via a feature flag in the console
wallet build configuration, where gRPC is not part of the default build.
Trying to run with the console wallet with the config option
`grpc_enabled = true` where the gRPC feature is not enabled for the
build will result in a runtime error.

Motivation and Context
---
See #5930

How Has This Been Tested?
---
Existing unit and cucumber tests pass

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

<!-- 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
---

- [ ] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [x] Other - The console wallet must be build with the `features =
["grpc"]` to enable gRPC

<!-- 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
Assignees

@hansieodendaal hansieodendaal

Labels
release-blocker Something that needs to be fixed before a release can be made
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@SWvheerden @CjS77 @hansieodendaal