feat: add user_identity to ShareAccounting

[average-gary]

Channels are aware of user_identity, this Draft adds a field to ShareAccounting populated by the user_identity from the channel.

Since this is related to #1902, I am marking Draft for now since I would have to refactor #1902 to accommodate these changes.

[GitGab19]

After a quick look, it looks good to me.

What I would add is a set_user_identity method for both client and server versions.

The client one could be definitely useful for the tProxy.

[plebhash]

tbh I don't really understand the motivation here

ShareAccounting is already a struct member of ExtendedChannel and StandardChannel, for which user_identity is also a struct member, both on client and server sides

so there's already a 1:1 relationship between each instance of ShareAccounting and its corresponding user_identity, and adding one inside the other feels a bit redundant?

am I missing something related to #1902 that's not yet present here nor there?

[average-gary]

am I missing something related to #1902 that's not yet present here nor there?

Yes, currently ShareAccounting has no data to identify what the user the share is for. The only identifying information is Channel Id, which when persisted, has no meaning without additional context.

[plebhash]

FWIW, I don't think ShareAccounting should be redundantly holding channel_id either.

AFAIU you'd need channel_id + user_identity for Persistance::persist_event, which is called during ShareAccounting::update_share_accounting

why not simply modify ShareAccounting::update_share_accounting so that it also takes channel_id and user_identity?

the context in which ShareAccounting::update_share_accounting is called already has that information available (as self.channel_id and self.user_identity, where self is Standard / ExtendedChannel)

which comes back to my original point:

having the same data living inside both Standard / ExtendedChannel AND ShareAccounting is redundant.

[plebhash]

What I would add is a set_user_identity method for both client and server versions.

The client one could be definitely useful for the tProxy.

@GitGab19 under which circumstances would set_user_identity be called?

Once a channel is instantiated (be it server or client side), it never really changes the user_identity across the lifespan of such channel.

For example, there's no set_user_identity method for Standard / ExtendedChannel (client or server side), simply because there's no SetUserIdentity message in Sv2 spec. It's defined in OpenExtended/StandardMiningChannel and that's it.

So assuming ShareAccounting would also hold this information (which I still think it's unnecessary), why would it ever need to be changed after initialization?

---

edit: everything above also applies to channel_id

[GitGab19]

@GitGab19 under which circumstances would set_user_identity be called?

I initially thought about using it to later set the right Sv1 username which is sent by the Sv1 miner in the mining.authorize message.

But given we need to open the channel before getting the mining.authorize message (to properly answer with the extranonce1 and extranonce2_size to the mining.subscribe) the upstream end of the channel opened would still contain the initial user_identity used when bootstrapping the channel.

I'll have to think more about this, to understand which could be the best thing here. For now just ignore my previous comments about this.

[average-gary]

Closing since I'll cherry pick and add to #1902 per #1902 (comment)