update fog and fog clients for encrypted memo changes: recoverable transaction history

[cbeck88]

This makes fog ingest include the encrypted memo in the TxOutRecord
object. It then passes through fog view as usual. We also update
the FogTxOut object to parse the memo out of the view response.

fog sample paykit is updated to both produce memos on new transactions,
and parse memos from incoming transactions, and remember the last one or an error.

This uprevs mobilecoin submodule, and makes the android bindings
and libmobilecoin able to build.

It installs a RTHMemoBuilder when those bindings create transaction
builder, but it does not configure the RTHMemoBuilder, because
we probably need to extend the bindings to do that properly.

It also gets all the tests passing

Still TODO:

• Make sample paykit actually read the memos and expose this info in some basic way
• Make fog test client confirm that the memos are working (if A sends to B then B got the right memo and so did A)

[cbeck88]

@voloshyn @mobilecoinfoundation/sdk Adam Mork,

let me know if you guys agree with the changes in this file, I think this is the right thing to do (remove the UnwindSafe bound that was on all of these functions), I have tried to explain why I did this.

If I don't do this, I get build errors like this:

error[E0277]: the type `(dyn MemoBuilder + Send + Sync + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   --> libmobilecoin/src/transaction.rs:343:5
    |
343 |     ffi_boundary_with_error(out_error, || {
    |     ^^^^^^^^^^^^^^^^^^^^^^^ `(dyn MemoBuilder + Send + Sync + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
    | 
   ::: libmobilecoin/src/common/boundary.rs:38:15
    |
38  | pub(crate) fn ffi_boundary_with_error<R, I>(
    |               ----------------------- required by a bound in this
39  |     out_error: FfiOptMutPtr<FfiOptOwnedPtr<McError>>,
40  |     f: impl (FnOnce() -> Result<R, LibMcError>) + UnwindSafe,
    |                                                   ---------- required by this bound in `boundary::ffi_boundary_with_error`
    |
    = help: within `std::option::Option<TransactionBuilder<FogResolver>>`, the trait `RefUnwindSafe` is not implemented for `(dyn MemoBuilder + Send + Sync + 'static)`
    = note: required because it appears within the type `*const (dyn MemoBuilder + Send + Sync + 'static)`
    = note: required because it appears within the type `Unique<(dyn MemoBuilder + Send + Sync + 'static)>`
    = note: required because it appears within the type `Box<(dyn MemoBuilder + Send + Sync + 'static)>`
    = note: required because it appears within the type `std::option::Option<Box<(dyn MemoBuilder + Send + Sync + 'static)>>`
    = note: required because it appears within the type `TransactionBuilder<FogResolver>`
    = note: required because it appears within the type `std::option::Option<TransactionBuilder<FogResolver>>`
    = note: required because of the requirements on the impl of `UnwindSafe` for `mc_util_ffi::FfiMutPtr<'_, std::option::Option<TransactionBuilder<FogResolver>>>`
    = note: required because it appears within the type `[closure@libmobilecoin/src/transaction.rs:343:40: 383:6]`

which I think cannot be resolved if we want to have Box<dyn ...> in the transaction builder, which I think we do.

[cbeck88]

@the-real-adammork ^

[cbeck88]

this has passed CD

[eranrund]

this has passed CD

Hows grafana looking?

[eranrund]

We don't update the fog sample paykit to actually read the memos,
but it does use the RTH memo build.

Is that not fog/sample-paykit/src/cached_tx_data/memo_handler.rs?

[eranrund]

Looks good as far as I can tell, although I am wondering about the decision to change the SDK bindings to use a partially-configured RTHMemoBuilder instead of the default one.

[eranrund]

Nice

[cbeck88]

I need to update the PR comment, the first commit didn't have the memo handler

[cbeck88]

Looks good as far as I can tell, although I am wondering about the decision to change the SDK bindings to use a partially-configured RTHMemoBuilder instead of the default one.

the thing is that to fully configure it, you need a SenderMemoCredential so that it can produce the hmac value, but to do that, we need the account key in scope probably, so we would have to change the bindings to do that. so i figured it's better to just hold off on that for now

[cbeck88]

@eranrund i will post grafana graphs shortly

[jcape]

LGTM FWIW.

I'm a bit concerned about catch unwind changes---like, it seems like the jni crate gets around this kind of thing by just going ahead and wrapping everything in Arc<Mutex<Foo>> and passing pointers to that, and is just highlighting that we're letting the freak flag fly (in a bad way) here.

It's not for this PR, of course, but maybe worth looking into as safety improvement work.

[cbeck88]

@jcape the error says this:

error[E0277]: the type `(dyn MemoBuilder + Send + Sync + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
   --> libmobilecoin/src/transaction.rs:343:5
    |
343 |     ffi_boundary_with_error(out_error, || {
    |     ^^^^^^^^^^^^^^^^^^^^^^^ `(dyn MemoBuilder + Send + Sync + 'static)` may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
    | 

So is Arc<Mutex<X>> going to fix that? Arc<Mutex<X>> can also contain interior mutability I think

[cbeck88]

@jcape i have read some more about this -- i think the best alternative I see here is, add an UnwindSafe trait bound to the MemoBuilder constraints in TransactionBuilder, to allow that people can use catch_unwind around transaction builder.

I don't think we should use Arc<Mutex<MemoBuilder>>, it just seems like an overkill. Also Mutex<MemoBuilder> introduces a dependency on the standard library threading stuff, which currently we don't actually have in transaction-std I think

[cbeck88]

rust-lang/rust#40628 (comment)