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

refactor mobilecoind api to not depend on consensus-api #3307

Merged
merged 8 commits into from
Apr 10, 2023

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Apr 9, 2023

I am having a lot of build problems in another project due to mc-mobilecoind-api pulling in mbedtls, which exports some symbols that conflicts with another C library that some rust code depends on.

(A grpcio thread crashes, causing the next thread to make a grpc request to hang:

023-04-09T22:21:35.457700Z  INFO mobilecoind_buddy::worker: check balance: 1
2023-04-09T22:21:35.458247Z  INFO mobilecoind_buddy::worker: check balance: 0
[New Thread 0x7ffff4c30640 (LWP 86787)]
thread '<unnamed>' panicked at 'Called self-test rand without enabling self-test', /home/chris/.cargo/git/checkouts/rust-mbedtls-12ed44423be981f0/ac6ee17/mbedtls/src/self_test.rs:52:20
stack backtrace:
[New Thread 0x7fffdca49640 (LWP 86788)]
[New Thread 0x7fffd100d640 (LWP 86789)]
[New Thread 0x7fffd080c640 (LWP 86790)]
[New Thread 0x7fffca690640 (LWP 86791)]
[New Thread 0x7fffc9e8f640 (LWP 86792)]
   0: rust_begin_unwind
             at /rustc/a266f11990d9544ee408e213e1eec8cc9eb032b7/library/std/src/panicking.rs:579:5
   1: core::panicking::panic_fmt
             at /rustc/a266f11990d9544ee408e213e1eec8cc9eb032b7/library/core/src/panicking.rs:64:14
   2: core::panicking::panic_display
             at /rustc/a266f11990d9544ee408e213e1eec8cc9eb032b7/library/core/src/panicking.rs:147:5
   3: core::panicking::panic_str
             at /rustc/a266f11990d9544ee408e213e1eec8cc9eb032b7/library/core/src/panicking.rs:131:5
   4: core::option::expect_failed
             at /rustc/a266f11990d9544ee408e213e1eec8cc9eb032b7/library/core/src/option.rs:2055:5
   5: rand
   6: _ZN9grpc_core12BdpEstimator12CompletePingEv
   7: _ZL22finish_bdp_ping_lockedPvP10grpc_error
   8: _Z31grpc_combiner_continue_exec_ctxv
   9: _ZN9grpc_core7ExecCtx5FlushEv
  10: _ZL10end_workerP12grpc_pollsetP19grpc_pollset_workerPS2_
  11: _ZL12pollset_workP12grpc_pollsetPP19grpc_pollset_workerN9grpc_core9TimestampE
  12: _ZL7cq_nextP21grpc_completion_queue12gpr_timespecPv
  13: grpcio::env::poll_queue
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
[New Thread 0x7fffc968e640 (LWP 86793)]
[Thread 0x7ffff6242640 (LWP 86784) exited]
[New Thread 0x7fffc8e8d640 (LWP 86794)]
2023-04-09T22:21:35.558976Z  INFO mobilecoind_buddy::worker: worker polling loop
2023-04-09T22:21:35.559023Z  INFO mobilecoind_buddy::worker: check ledger status

)

I determined that the only reason I have mbedtls in my build is that mobilecoind-api depends on consensus-api and this pulls in mc-attest-core etc.

This change allows me to break this dependency.


I think this change is good from a separation of concerns point of view anyway. Consensus should be able to evolve its API without worrying if it's breaking mobilecoind users.

I am having a lot of build problems in another project due to
mc-mobilecoind-api pulling in mbedtls, which exports some symbols
that conflicts with another C library that some rust code depends on.

I determined that the only reason I have mbedtls in my build is
that `mobilecoind-api` depends on `consensus-api` and this pulls
in `mc-attest-core` etc.

This change allows me to break this dependency.
@cbeck88 cbeck88 requested review from jcape, eranrund, a team and awygle and removed request for a team April 9, 2023 22:37
Copy link
Collaborator

@nick-mobilecoin nick-mobilecoin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good as long as we're OK with the two LastBlockInfo versions possibly deviating.

This specific dependency issue will hopefully go away with mobilecoinfoundation/mcips#64, but that's a ways out for this current problem

mobilecoind/api/proto/mobilecoind_api.proto Show resolved Hide resolved
mobilecoind/api/proto/mobilecoind_api.proto Show resolved Hide resolved
mobilecoind/src/service.rs Show resolved Hide resolved
Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change to the mobilecoind API, so it should be noted in the commit message (or a ChangeLog added), and the reserved 2 issue @nick-mobilecoin identified should be fixed. I'm ambivalent about the change itself.

What impact does this have on mobilecoind-json, if any?

@cbeck88
Copy link
Contributor Author

cbeck88 commented Apr 10, 2023

I dont think its a breaking change as long as new and old structs are wire compatible (the reserved 2 thing is fine though)

@cbeck88
Copy link
Contributor Author

cbeck88 commented Apr 10, 2023

I dont think this struct is exposed through mobilecoind-json, we generally dont add new features to that.

@jcape
Copy link
Contributor

jcape commented Apr 10, 2023

Deleting deprecated fields and methods is always a breaking change :-)

@cbeck88 cbeck88 requested a review from jcape April 10, 2023 16:28
@cbeck88 cbeck88 enabled auto-merge (squash) April 10, 2023 20:10
@cbeck88 cbeck88 merged commit c641991 into master Apr 10, 2023
@cbeck88 cbeck88 deleted the refactor-mobilecoind-api branch April 10, 2023 20:50
NotGyro pushed a commit that referenced this pull request Apr 10, 2023
* refactor mobilecoind api to not depend on consensus-api

I am having a lot of build problems in another project due to
mc-mobilecoind-api pulling in mbedtls, which exports some symbols
that conflicts with another C library that some rust code depends on.

I determined that the only reason I have mbedtls in my build is
that `mobilecoind-api` depends on `consensus-api` and this pulls
in `mc-attest-core` etc.

This change allows me to break this dependency.

* remove stuff from build.rs that isn't needed anymore

* cargo lock

* fixup

* fixup

* restore deprecated field (avoid compatibility break)

* add changelog entry

* fixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants