any chance for 8617 (background sync) to make it in next release/

s///?/

r4v3r23[m]: it's a large PR that isn't code reviewed yet, only tested

won't make it in v0.18.2.0

Y'all, take a look at monero-project/monero #8707#issuecomment-1382686284. We may need to roll back

jeffro256[m] no need to roll back, the current code is correct because it doesn't use the cache for CLSAG

selsta: Any target date for the next release?

IIRC Not earlier than February

The current code happens to not have a double spend bug because of what kayabanerve[m] said. In the one place that calls verRctNonSemanticsSimpleCached, the message field is populated as a hash of the transaction prefix which contains the key image, which is the data reconstructed into rctSig::p::CLSAGs[x]::I

sech1 BP+ still uses CLSAG

The RCT Type CLSAG is for CLSAG + BP.

jeffro256: We don't need to roll back

Despite reading the comments it was serialized, I left my sign off clear it was only if someone else explicitly commented. I wouldn't screw around here

Yes I overreacted. We're safe but not because of inherent properties of the new function

I think this is too narrow of a margin for me to be comfortable

So while I'm extremely annoyed this test wasn't in the PR, and our safety is on a fragile indirection, and concerned over how the original PR which cut out almost all checks on a fragile cache, I ack the obvious perf benefits justifying its presence and want to move forward. That means having this test now and improving how we define cache keys.

My original suggestion was full net serialization + RCT serialization.

In the issue, I posited 4 solutions, one being simply explicitly transcripting key images, or editing do_serialize (I don't believe we can do the latter though).

I think it's better to explicitly serialize what was left out in BEGIN_SERIALIZE_OBJECT/END_SERIALIZE in rctTypes.h

If our serialization perf isn't atrocious, I return to saying full net serialization + RCT serialization. If there are perf concerns over that, then transcripting just the key images explicitly.

That would require a hard fork IIUC

sech1 I hate the idea of bloat and yell at devs I work with all the time. Why the hell do they want string types when the perfectly optimal bytes are right there .-.

mgSig.II, clsag.I, Bulletproof.V - these 3 are not serialized by default

But despite the 'bloat' of doing a full net serialization

It's a complete answer to every single discussion here

and lets us not consider this a fragile monstrosity removing every single check on the protocol

I can add serialization of these 3 to the cached version

... we don't serialize BP V? Do we serialize BP+ V? Doesn't this allow breaking range proofs if not for the message indirection?

/ Commitments aren't saved, they're restored via outPk

this is what it says about BP V

... right, but are they in do_serialize or not

BP+ V is not serialized as well

kayabanerve Doing a full net serialization still wouldn't catch this issue IIUC since the .I field isn't serialized anyways

If they're not, fuck it, serialize the entire TX. I'm not signing off on anything else at this point. Y'all don't need my sign off, but this is insane that we didn't introduce double spends thanks to an indirection and didn't allow breaking the range proofs thanks to the same indirection.

jeffro256: TX serialize

TX net serialize + RCT internal serialize

K. I made it explicitly clear I only gave my approval of this PR dependent on do_serialize being PERFECTLY comprehensive. We have now proven it wasn't, with key images, and sech1 is further claiming it doesn't handle BP values either.

I'd never have signed off on the original PR under these conditions. I don't want to play a game of cat and mouse on fields we do/don't serialize. The net TX has everything except the mix ring, which we get via do_ser and test for.

kayabanerve[m] ah I get you

"This isn't a cat and mouse!" I thought it was everything, jeffro proved it wasn't key images, and now it also isn't BP+ V

Since this literally disables almost every check in the protocol, I don't want to play around here and I'm done believing things which aren't correct. We save 5x. We can bear the mem cost of doing the serialization.

I'm updating the original PR and the test with my stance. I hope someone makes a PR updating this or I'll make one myself.

I'll make a PR that adds serialization of all missing fields to the cached version

I take some responsibility for this since I insisted the serialization was comprehensive. It was not.

By double spend you guys don't mean create coins out of nothing, right? Just messing with nodes or wallets or whatever to make them believe they got coins

there's no double spend even with this PR, right kayabanerve[m] ? Because it's called from a place where missing fields are already filled in with correct data

If this issue could be exploited it should be detectable by a node with fixed rules

sech1: Your PR is only secure by an indirection

That's the frustration here

if these fields are not serialized, there's no way for an attacker to modify them and send modified tx

exactly because they are not serialized

jeffro256: I also saw vtnerd cited. I'm not trying to blame anyone for that misunderstanding. I'm trying to channel this frustration into progress and appreciate proper testing.

monerobull: It's a commentary on proximity, not risk, please don't take this out of context.

(if this code was slightly different, it would be one. It's not slightly different. We're discussing edits so it's much harder to fall into the broken category)

@sech1 No

no wait, there's a second serialization function

such a mess

An attacker can submit TX A with valid proofs, then TX B with the same proofs yet different images under your PR if images weren't in the message hash

^^^^^^^

But because images are in the message hash, which is serialized by do_serialize, they cannot

I approved the PR on the premise images would be explicitly serialized by do_serialize

They are not

jeffro256: proved that with their test

I'm personally pissed I approved a PR on a premise that's faulty, only securing our entire blockchain by an indirection which is part of the monolithic protocol we're tearing apart with Seraphis

Thanks for bringing this up on github btw kayabanerve[m] on making me reconsider and write that test lol

u were persistent good job

So I'm appreciating jeffro256 's test, and calling for an edit to the cache key transcription since this code *disables almost all checks on a belief of prior execution*

Yet if it wasn't for the message indirection, that'd be an incorrect assumption

So while we don't have a CVE/need to revert thanks to the indirection, I'm no longer playing cat and mouse with ser/do_ser/manually adding fields. I'm personally calling for a cache key of serialize(tx) || do_serialize(rct) with the RCT tests confirming mixRing inclusion

Which is what jeffro256 just wrote a test for. They're also testing additional properties, yet with serialize(tx) to start the cache key, those will no longer be needed due to how complete a solution this is

I fully understand the distinct serialization functions. The issue is the distinct serialization function, meant to be complete, is not.

"serialize(tx) || do_serialize(rct)" what does it mean?

Also, to be clear, I commented on the original PR so all prior participants got pinged.

sech1: You used hash(do_serialize(rct)) as a cache key

I'm calling for hash(serialize(tx) concat do_serialize(rct))

So literally, the entire over the wire blob, with everything except the mix ring, and then the locally expanded RCT, including the mix ring, as tested by jeffro256

no, I asked about "serialize(tx)"

do_serialize did not include key images. We at least should do key images || do_ser(rct) as a cache key. Except you said V also isn't included and I don't want to play cat and mouse, finding field after field for years to come on code disabling all cryptographic checks

what serialize function specifically?

... convert the entire TX to bytes for the P2P layer?

I believe it's just ::serialize

I'm actually unsure.

there is serialize_rctsig_base and serialize_rctsig_prunable

... I'll say whatever the RPC uses for the binary TX format. That's what I believe is sent over P2P as well, the default serialization mode of Monero, and what I've always impl'd in my libs

I think both should be called

sech1: I'm talking about the entire TX

Not just RCT

This would make verifyRctCached take the entire TX, not just RCT

I don't disagree. Serializing the entire TX includes both. do_serialize also should, which is what we currently have. This is an unhelpful side discussion AFAICT

::serialize would just go through the same BEGIN_SERIALIZE/END_SERIALIZE macros that miss some field, right?

Please see the concat

It's serialize on the TX, missing the mix ring. It's do_serialize on the RCT, including the mix ring. That is complete.

jeffro256: Will you update your current PR or make a followup PR to update the cached transcription?

ahhhhh

(to AT LEAST key images || do_ser(rct), but I've made my preference/what I'll sign off on clear)

I'm slow today

Or should I make the followup PR

so you want verRctNonSemanticsSimpleCached to have the second parameter - tx, and serialize it too?

sech1: All good. I get to be stressed :p

Yeah I can write that

it doesn't need two since passing tx passes rct uinderneath it

It just needs rct -> entire TX

Thanks jeffro256. Let me know

::do_serialize(ar, tx);

::do_serialize(ar, tx.rct_signatures);

kayabanerve[m] like this? ^^^^

The former doesn't need to be do_ (not that I'm complaining) but yes. If you want to make the PR yourself, I'd also appreciate that :)

So long as someone does. I can also just do it myself lol. It's not a big PR. One line add :p

I just pointed to jeffro256 since they were already working on this with an open PR

wait, there's ::do_serialize and ::serialize? wtf

jeffro256[m] can you add it to your PR? To keep new tests and fixes together

yeah that was my reaction a couple weeks ago looking a cryptonote::transaction lol

Yeah of course

jeffro256: just rewrite it in rust already

:p

so the function should be changed to "bool verRctNonSemanticsSimpleCached(cryptonote::transaction& tx)"

and then ::do_serialize(ar, tx); ::do_serialize(ar, tx.rct_signatures); in the code of the function

With jeffro's test for mixRing inclusion in do_serialize (and the other pleasant goodies)

That ensures there's nothing we miss

Because just do_ser(rct) misses key images

Serializing the entire TX won't, yet will have relative output indexing. Hence...

output indexing?

but this cache is for the mempool

I was gonna use cryptonote::get_transaction_hash since that *should* contain serialization for transaction prefix + blob cache And its usually cached well

cryptonote::get_transaction_hash should be good enough too

Serializing the entire TX is insufficient. It only has relative output indexing, premised on the global chain state, communicated via offsets. Serializing RCT solves by that serializing mixRing. Serializing RCT has been found to be insufficient. Serializing the TX solves that.

BUT PLEASE DOUBLE CHECK

Uhhh TX hash, if the e2e hash, should be prefix, base, and prunable

Which would be everything

yes

Sorry I meant get_transactio_hash in addition to do_serialize(rctSig)

It uses the network serialization, not the signature serialization so... meets my goals 👍️

I assumed you meant g_t_h instead of ser(tx) ;) All good

so "hash(TX hash || do_serialize(rctSig))" will be used for caching, correct?

Yes

ok, now we're all on the same page

TX hash as in what we use on the explorer, like in the URL here: xmrchain.net/tx/8d28f4d4601c5a0c201…127bce61af7d2c5668b1cd59dffc24f52a3 ? If it's this hash, then it really must have everything in it

I think cryptonote::get_transaction_hash is this hash, correct me if I'm wrong

"hash(TX hash || do_serialize(rctSig))" is good then

yes, it calls calculate_transaction_hash and caches it internally, and I recognize calculate_transaction_hash's code - it's the same I use in p2pool and it gets tx hash that ends up on blockchain explorers

👍️

there is a flooding attack on testnet

No, it's one of p2pool users running a script to provide transactions

We'll be running tests next week

dEBRUYNE: p2pool will hardfork so target is beginning of February

correction: p2pool hardforks on March 18th. Beginning of February is when p2pool v3.0 binaries will be available to be included in new Monero release

I meant target for putting out the new monero release

jeffro256[m] kayabanerve[m] I'm testing this patch on my nodes now: paste.debian.net/hidden/1e016994 - you can add it as is to your PR, or maybe you already have your own solution

yeah hashing the tx id is the right way to go

Why am I getting [1673693423] libunbound[5274:0] error: read (in tcp s): Bad file descriptor for 8.8.8.8 port 53 error? Have I configured something incorrectly or is it a bug?

I use a binary compiled from the master branch with a back ping patch

And I launch it with DNS_PUBLIC="tcp://8.8.8.8"

I get this error about once a day

llacqie[m]: I have seen this a couple times but usually don't get it once per day

bad filedescriptor means somebody closed it without removing all references to it first

probably a race condition with an async close

that would most likely be a bug in libunbound

selsta: Thanks. As far as I read, it only concerns miners, not actual nodes, correct (re: the hard fork)?

yes, only p2pool miners

sech1 kayabanerve patch was pushed + a couple unit tests