Hi, just wondering if Multisig fixes are likely to be fixed before the hardfork?

johnfoss68[m]: that's the goal

<UkoeHB> "johnfoss68: that's the goal" <- Thanks!

jberman: hows background sync going?

The core logic in libwallet is written here: j-berman/monero 238ea53

It implements what I described in this comment: monero-project/monero #8082#issuecomment-1058967180

I plan to clean that up and add more tests. The final handoff to actually get it into wallets also will be a bit tricky I think. I'm prioritizing it behind hard fork stuff at this point, since it can be released whenever. I haven't done anything with it since this comment: m2049r/xmrwallet #785#issuecomment-1079569991

@hyc regarding your comment here: monero-project/monero #8272#issuecomment-1105163109, do you mean to say that some websites have an OpenSSL library that doesn't support MD5 and which the user who is compiling can't switch out / rebuild the library?

Because if you have the priviledge to compile and run arbitary c++ code, couldn't you just link your own choice of OpenSSL? And a lot of the other dependencies of monero don't come standard on most people's systems anyways, so they're already having to pick and choose which dev libraries they use

<jberman[m]> "It implements what I described..." <- cheers

jeffro256[m]: I mean some sites choose not to enable MD5 in their custom builds of OpenSSL. "site" = any particular deployment, not a website

we've run into this many times now with OpenLDAP, people complain that it doesn't link on their machine

because some cipher or hash algo we used isn't present on their OpenSSL

you're probably right that we should be using a modern auth mechanism like SCRAM-SHA256 or something instead

moneromooo: Have a minute?

I reverted the change, no issue, EXCEPT... this was coded in a really weird way.

So I initially removed the INV_8 code from BP+ before noticing BP also does everything INV_8, yet then libringct multiplies it by 8 to normalize it (normalize may not be the best term, but I'm going with it).

The weird part is BP+ verification assumes it's of form INV_8 and so does BP.

So for the existing BP, instead of allowing torsion (which I thought it would) or mul'ing by curve order to check (which I assumed it would), converts it to form INV_8 in expand transaction.

While I don't mind BP/BP+ being independent code breaking flow from the rest of XMR, my question becomes why do we do this in expand transaction instead of in libringct. We do the * 8 there with the result of BP_PROVE already.

Yet I'm hesistant to make this PR larger than a simple reversion of the existing code and move BP's existing * INV_EIGHT into libringct, even though I think that's the best move overall (as code being strewn about is what caused this long discussion in the first place as most of us missed a line in blockchaindb of all places).

I also don't know if breaking the API of expand_transaction like that has larger impacts (don't see why someone would hook into that yet not libringct but /shrug)

It looks like it's because expandTransaction moves rv.outPk into rv.p.bulletproofs already so that's just the optimal location. I'd honestly rather this entire loop be moved into libringct but technically this is expansion?

kayabanerve[m]: you shouldn't need to touch the bp+ file

just mimic the old bp pattern for bp+

UkoeHB: Yeah, I reverted all of those changes.

... right. I have that in front of me. I'm now asking if the old pattern was stupid and if expand_transaction shouldn't mul by INV_EIGHT yet ringct should.

¯\_(ツ)_/¯ Maybe for the scope of another PR then

iirc expand_transaction is doing 1/8 to initialize the bp's commitment vector (i.e. as part of deserializing)

It's doing 1/8 *when* it initializes the bp's commitment vector as part of expansion. That's the distinction I want to draw.

Yet considering it's initializing the bp's commitment vector from data in the signature already, this entire BP vector initialization can be moved into libringct

The alternative would be letting the bp structs wander around half-constructed.

I guess? I was hoping to scope it as nothing relying on this. Monero should never care about V, solely outPk.

even if technically you could do that and make it work, it is a serious code quality violation

One's the definitive definition for output values. One's an implementation detail of the range proof backend.

I'm actually arguing this is the code quality violation :p

But I'll ack it's a monolithic structure not worth changing now, the same argument I made to revert this optimization, and am just re-running the core tests before I submit this

I don't think so. The tx blob is a compact data format, and part of deserializing it into structs in initializing all code objects fully (which includes de-compacting the commitments via mul 1/8).

is initializing*

Fair. I can definitely see that point of view. I think my frustration is the duplication and comment that if the BP code does want this passed via their struct for whatever reason, libringct should handle that calling convention. It shouldn't be any concern of Monero as a whole which shouldn't even acknowledge this field's existence IMO.

As a customer of the bp objects, we have no choice but to care about their layouts. One viable alternative would be removing V from the structs, and passing them as inputs to the verifier.

That's what I decided to do for seraphis proofs (but of course it is open to debate as a design choice).

Right, yet we do have the choice of when to consider them relevant. Right now, that's in expand_transaction which moves from outPk (part of rv) to V (also part of rv). My comment is we could handle this purely in libringct (the end consumer of rv).

The only comment I would have otherwise is if Monero is expected to save V to the disk so it can be sent to others. It isn't sent to others. It's not serialized because it's derived from outPk :p

So when we have the choice of when to ack/work with it, and the actual serialized protocol doesn't ack this, and it's a forced on us calling convention... why are we spreading the code out? Is there any actual consumer of bp.V outside of libringct?

Yeah, that's how you get spaghetti - introducing complexities for the reader to A) think of in the first place, B) track down their consequences. Passing around half-initialized objects is a very strong vector for spaghetti.

... I'm saying the fact I missed this because we have crypto ops outside of crypto code is spaghetti :p

Like I literally didn't see this handled in libringct, missed that it did need to be handled, and spent 50 minutes on a failed test run because we have BP code outside of libringct.

When we have the first half of this, * 8 for representation in the TX, in libringct yet not its counterpart.

I do get the concern with half-initialized objects though. If we had multiple consumers, I'd agree. I just don't believe we have multiple consumers unless this field is written to the DB.

That's an argument to remove V from the structs, not to half-initialize objects.

Which I wouldn't know. I would assume not since it isn't serialized? Yet for all I know DB code doesn't use network serialization.

That's an argument this code is screwy no matter what and I'd appreciate the code for screwy code be in one file in one library instead of split between the lib expected to handle this, which is also the sole consumer, and the main Monero node code.

yeah see - complexity from figuring out who the consumers are

But also, sure, I wouldn't mind a PR to remove V from structs

But again, I do see how this is a complex monolith and it's not worth changing right now

Running the tests with expand_transaction handling this, just miffed I missed it and have my own opinions on design ;)

ok, but please don't half-initialize things in code I have to read... (and if anyone finds me doing this, call it out thanks)

Yeah, yeah, keeping it as expected

👍

IIRC I put it there so it was always as expected (ie, not pre-multiplied by 1/8).

If people prefer it elsewhere sensible, I have no objection really.

Setup on load seemed good.

ErCiccione selsta: meant to respond on sunday, yes I will be doing reviews

I kinda agree that the mul by 8 in blockchain_db is unfortunate though :)

vtnerd: 8220 is ready for review, 8149 can be re-reviewed (still needs one small rebase that will change ~10 lines though)

moneromooo: It seems like koe agrees :p If this does change, it should by removing it out of the struct into a call argument. Anyways. PR with just the reversion is out :)

thanks kayabanerve[m]

Happy to