add request type in broadcast message

[Amir-m-a]

Added index of request type (execution/validation) as an argument with default parameter to clarify deserialization of std::variant. Also removed the callback of write since it is not an error handling callback!

[HalosGhost]

Thanks for jumping in! Am I understanding correctly that this is intended to update -js to make it easier to support sentinel attestations?

[Amir-m-a]

Sorry for delay, wanted to make working example and it took more time than expected.

So basically I tried -js with a running -tx with 2PC arch and it didn't work (and I wonder if it ever did in the past). There was no response from the sentinel after sending a valid tx.

After comparing the request with the client in -tx I found that a single byte was missing from -js request. The following lines were the cause of the issue as std::variant is serialized as index of type + type:

using request = std::variant<execute_request, validate_request>;
using response = std::variant<execute_response, validate_response>;

The first commit in this PR makes the -js work and if reqType is set to 1 it will also get the attestation as you mentioned.

The second commit (hopefully) properly reads the response from sentinel (or coordinator or shard). It also adds compact tx representation and make it possible to mint and transfer solely with -js (as it's shown in README example).

[HalosGhost]

Sorry for delay, wanted to make working example and it took more time than expected.

Not a problem!

So basically I tried -js with a running -tx with 2PC arch and it didn't work (and I wonder if it ever did in the past). There was no response from the sentinel after sending a valid tx.

This makes sense actually; -js is only semi-maintained (in the sense that we're happy to review PRs and if there's interesting work to be done on it, we'll happily lend a hand, but our attention is pulled in too many directions to proactively keep it up-to-date, unfortunately). Since sentinel attestations were added to -tx, the message formats that sentinels interpret changed, and -js hasn't been updated to handle them.

The first commit in this PR makes the -js work and if reqType is set to 1 it will also get the attestation as you mentioned.

The second commit (hopefully) properly reads the response from sentinel (or coordinator or shard). It also adds compact tx representation and make it possible to mint and transfer solely with -js (as it's shown in README example).

This sounds terrific, thank you for the help! I'll work to get a t-ACK for you today!

[HalosGhost]

heads-up in-advance; OpenCBDC uses the DCO, and all commits need a sign-off before merging.

Just in-case it's not clear, the intent of this is to ensure you keep your intellectual property rights for your contributions!

[HalosGhost]

concept-ACK. Looks like there a couple small clean-ups/fixes to go through first, but my impression is that this is a solid contribution headed towards bringing -js inline with supporting sentinel attestations.

I do have one question for you though: this change also adds the ability to communicate with the coordinator and shards. Do you think that's missing or useful functionality?

It was previously left out because there's effectively no reason to allow communication to any component other than the sentinel (and, perhaps the watchtower); presumably, -js would be most useful for external implementers/integrations—users who would be explicitly outside the secure perimeter.

[HalosGhost]

All these instances of (!str) in obj are likely incorrect. (Note though that this was actually already wrong: ! binds tighter than in.)

These probably all need to be corrected to be of the form !(str in obj) such that it is negating the property-existence-check (rather than negating the string before checking that it exists in the object).

These checks as-written will probably never trigger (again, they probably never would have before either), as !str in obj (which is equivalent to (!str) in obj) effectively evaluates !str to false which is then coerced back into a string ('false'), and 'false' in obj is likely to return false in most cases.

[HalosGhost]

This snippet (and, perhaps, more generally code to validate a message is well-formed) appears in enough places, it probably makes sense to be turned into a helper (even if not exposed external of this file/module).

[Amir-m-a]

added DCO.

Do you think that's missing or useful functionality?

In case of coordinator, I did it mostly to mint without using -tx client, but as you mentioned minting can be limited to trusted and secure perimeter and doesn't need exposed API.
In case of shard though I think it might be useful to be able to check validity of a token or status of tx. As sentinels can only statically check tx, one can't dry-run a tx or check if an imported utxo is ideed unspent.

All these instances of (!str) in obj are likely incorrect.

I felt it's wrong but considered it as yet-another-weird js syntax and didn't dig into it, sorry. I starting fixing them and added a helper function but on second though are these checks really necessary? I mean one can't simply make an Input for example without defining value property unless intentionally removing it which is definitely not a case that needs to be checked IMO. Also if the properties are undefined it will lead to exceptions while converting to buffer anyway. As I said I'm fine fixing them, and I mostly did it already, just wanted to ask if it's better to remove these checks instead and add some other sanity checks that might be more useful like checking length of provided hex values, value ranges, etc.

[HalosGhost]

added DCO.

Thank you!

In case of coordinator, I did it mostly to mint without using -tx client, but as you mentioned minting can be limited to trusted and secure perimeter and doesn't need exposed API. In case of shard though I think it might be useful to be able to check validity of a token or status of tx. As sentinels can only statically check tx, one can't dry-run a tx or check if an imported utxo is ideed unspent.

This is an interesting point, but I think freely exposing communication to internal components is an unacceptable solution (this is not intended as mean!). There are two reasons for this, one general, and one shard-specific:

1. In general, access to inside-the-sentinel-barrier components freely allows the communicator to undetectably inflate or deflate the monetary supply. This is a core part of the -tx threat-model (which we're actively engaging in work to try and weaken for better ergonomics and security guarantees). In the meantime though, it means that all messages which are expected to maintain balance are absolutely required to go through the sentinel layer (and this is also why it is assumed and required that minting/redemption must happen within the sentinel layer, since those transactions fundamentally alter balance).
2. Shard-specific though, you cannot reliably interact with a single shard and gain useful insight about the system. You must interact with all shards to get a reliable view of the UHS (or the relevant subset to get a reliable view on how a particular transaction would proceed). To that end, there's effectively no definable role in the current threat model which can sensibly talk to shards other than the orchestration component (either the atomizer or the coordinator).¹ Practically speaking, in the current threat model and implementation, this means clients ability to talk directly to shards serves no useful purpose other than testing.

To that end, I think enabling communication to internal components can be useful in specific circumstances—predominantly testing—(and I'm not against merging it), but I think it should be gated by a global-unsafe-declaration flag react-style, and must print a diagnostic to the log saying something to the effect of “[SECURITY VIOLATION]: Bypassing sentinel layer is enabled!” on every invocation of sending a message.

The alternative is to expose an additional API call for the sentinels to support (e.g., “is this note spendable?”), and to then expose that interface through clients (and, by extension, this library). This is quite a bit more work, but that's how it must be done to not open a massive security violation.

All these instances of (!str) in obj are likely incorrect.

I felt it's wrong but considered it as yet-another-weird js syntax and didn't dig into it, sorry.

Pretty obviously, I missed it in the initial code review before release; don't sweat it!

I starting fixing them and added a helper function but on second though are these checks really necessary? I mean one can't simply make an Input for example without defining value property unless intentionally removing it which is definitely not a case that needs to be checked IMO. Also if the properties are undefined it will lead to exceptions while converting to buffer anyway. As I said I'm fine fixing them, and I mostly did it already, just wanted to ask if it's better to remove these checks instead and add some other sanity checks that might be more useful like checking length of provided hex values, value ranges, etc.

Good point! I'm quite comfortable with more useful sanity checks being put in-place instead; but it might be helpful context to know why these checks are here in the first place.

In -tx, which is very much research-grade code, the components do very naïve and trusting deserialization of messages. So much so, if you send them malformed messages, most cases will simply result in the component crashing. This is a trivial DoS vector and should absolutely be fixed in any project aiming for productionization. However, for our purposes, it was much simpler to just double-check we are sending valid messages than to actually gracefully handle malformed messages (despite it obviously being unacceptable in-production).

So, I think it might be best if we leave the current checks in-place, suitably updated to function correctly; and, if you'd like to either extend this PR with better sanity checks that enforce useful properties rather than the kind-of-silly format-compliance ones (or open that as an additional PR), they can be added.

Does that seem like a reasonable plan?

Footnotes

1. However, if the shard's duty is expanded (say, to include a mapping of UHS IDs to proofs which enable more interesting information to be gleaned from the UHS, as in the privacy-preserving auditing explorations we've been doing), there may be other components who can sensibly interact with shards (but again, only by talking to all relevant shards to whatever question is being asked). ↩

[HalosGhost]

Requesting an additional review (particularly making sure I've not totally missed anything with my most recent post/analysis) from @anders94

[Amir-m-a]

exposing communication to internal components is an unacceptable solution

I totally agree with you here and understand two reasons you provided, enabling -js to freely interact with shards/coordinator is like having another version of sentinel, not a tool to interact with it and to satisfy any requirements it's better to modify sentinel itself.

Thing is a supervisee of mine is working on -ui and it needs to mint tx (for admin panel) and check validity of tokens. In that context, -js is running as a back-end component, so it is in a more trusted perimeter than for example browser of user. In the current state, that the only way to mint or dry-run is to bypass sentinel, I couldn't come up with any other solution than modifying -js to make a working -ui.

All in all I am convinced to mark communicating with non-sentinel components as test-only or DON'T-USE-unless-your-life-depends-on-it, yet I have no idea how to do it as it is not made explicit what the destination endpoint is! Maybe just making it clear in jsdoc or README is enough?

One minor point is (maybe I am wrong here) the function was named broadcastTx which gave the idea that it (in its complete form) was supposed to send requests to group of endpoints and gather responses. After that discussion though maybe renaming it to sendToSentinel is better.

Does that seem like a reasonable plan?

Sure! Moved the fixed version of previous checks to helper function and added hex character and length checks. Still there's always room for more checks ...

[HalosGhost]

That's very exciting, and it's very cool to hear why you're poking around with this.

I think I might suggest two specific options then:

1. keep broadcastTx as it was before, and add a newly exposed method called mintTx. This function would be the one you'd want to warn on, and you can warn on it regardless of endpoint since it's a privileged operation anyway. This also makes it much more clear where the problematic use is! To that end, having a global boolean with a scary name can be the choice between simply erroring out on calling mintTx, or warning and continuing. [Edit to add]: This also assumes that broadcastTx is left only useful for sending a signed, full transaction to a sentinel, perhaps asserting the inputs are non-empty. This wouldn't prevent a malicious caller from simply bypassing this restriction with a copy of the code, but we shouldn't encourage it.
   Note: the naming doesn't feel problematic to me because the client wouldn't otherwise send anything else to any component besides a sentinel.
2. Expose a method for importing coins/keys generated by the -tx reference client cli

These are effectively the two options I laid out in opencbdc-ui#2; it sounds like you may have already taken a look at it since it sounds like you're looking to add an admin panel.

I'd be quite comfortable with either option (or both, if there's a benefit to it)!

From the perspective of -ui serving as a non-custodial intermediary, the ability to import coins (and not keys!) seems the most sensible; but from an ease-of-testing perspective, the ability to mint coins directly is clearly much simpler.

When you're ready for me to take another walk through the code, feel free to re-request my review. (And I'll continue to monitor discussion and help plan and troubleshoot!)