03:15:11 -xmr-pr- jeffro256 opened pull request #8707: unit_tests: do_serialize() assumption in verRctNonSemanticsSimpleCache...
03:15:11 -xmr-pr- > https://github.com/monero-project/monero/pull/8707
05:58:09 <r4v3r23[m]>  any chance for 8617 (background sync) to make it in next release/
05:58:14 <r4v3r23[m]> s///?/
06:02:09 <selsta> r4v3r23[m]: it's a large PR that isn't code reviewed yet, only tested
06:02:16 <selsta> won't make it in v0.18.2.0
08:02:30 <jeffro256[m]> Y'all, take a look at https://github.com/monero-project/monero/pull/8707#issuecomment-1382686284. We may need to roll back 
08:44:07 <sech1> jeffro256[m] no need to roll back, the current code is correct because it doesn't use the cache for CLSAG
08:57:46 <dEBRUYNE> selsta: Any target date for the next release?
09:02:15 <sech1> IIRC Not earlier than February
09:25:27 <jeffro256[m]> 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
09:25:53 <kayabanerve[m]> sech1 BP+ still uses CLSAG
09:26:09 <kayabanerve[m]> The RCT Type CLSAG is for CLSAG + BP.
09:26:26 <kayabanerve[m]> jeffro256: We don't need to roll back
09:26:54 <kayabanerve[m]> 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
09:26:59 <jeffro256[m]> Yes I overreacted. We're safe but not because of inherent properties of the new function
09:27:41 <jeffro256[m]> I think this is too narrow of a margin for me to be comfortable
09:28:08 <kayabanerve[m]> 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.
09:28:24 <kayabanerve[m]> My original suggestion was full net serialization + RCT serialization.
09:29:03 <kayabanerve[m]> 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).
09:29:46 <sech1> I think it's better to explicitly serialize what was left out in BEGIN_SERIALIZE_OBJECT/END_SERIALIZE in rctTypes.h
09:29:48 <kayabanerve[m]> 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.
09:30:16 <jeffro256[m]> That would require a hard fork IIUC
09:30:30 <kayabanerve[m]> 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 .-.
09:30:37 <sech1> mgSig.II, clsag.I, Bulletproof.V - these 3 are not serialized by default
09:30:47 <kayabanerve[m]> But despite the 'bloat' of doing a full net serialization
09:30:53 <kayabanerve[m]> It's a complete answer to every single discussion here
09:31:05 <kayabanerve[m]> and lets us not consider this a fragile monstrosity removing every single check on the protocol
09:31:15 <sech1> I can add serialization of these 3 to the cached version
09:31:39 <kayabanerve[m]> ... we don't serialize BP V? Do we serialize BP+ V? Doesn't this allow breaking range proofs if not for the message indirection?
09:31:51 <sech1> / Commitments aren't saved, they're restored via outPk
09:32:01 <sech1> this is what it says about BP V
09:32:02 <kayabanerve[m]> ... right, but are they in do_serialize or not
09:32:19 <sech1> BP+ V is not serialized as well
09:32:23 <jeffro256[m]> kayabanerve Doing a full net serialization still wouldn't catch this issue IIUC since the .I field isn't serialized anyways 
09:32:36 <kayabanerve[m]> 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.
09:32:40 <kayabanerve[m]> jeffro256: TX serialize
09:32:49 <kayabanerve[m]> TX net serialize + RCT internal serialize
09:33:27 <kayabanerve[m]> 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.
09:34:09 <kayabanerve[m]> 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.
09:34:09 <jeffro256[m]> kayabanerve[m] ah I get you
09:34:34 <kayabanerve[m]> "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
09:35:10 <kayabanerve[m]> 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.
09:35:37 <kayabanerve[m]> 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.
09:37:11 <sech1> I'll make a PR that adds serialization of all missing fields to the cached version
09:37:11 <jeffro256[m]> I take some responsibility for this since I insisted the serialization was comprehensive. It was not.
09:39:05 <monerobull[m]1> 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
09:40:23 <sech1> 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
09:40:23 <jeffro256[m]> If this issue could be exploited it should be detectable by a node with fixed rules
09:40:52 <kayabanerve[m]> sech1: Your PR is only secure by an indirection
09:41:00 <kayabanerve[m]> That's the frustration here
09:41:04 <sech1> if these fields are not serialized, there's no way for an attacker to modify them and send modified tx
09:41:13 <sech1> exactly because they are not serialized
09:41:30 <kayabanerve[m]> 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.
09:41:42 <kayabanerve[m]> monerobull: It's a commentary on proximity, not risk, please don't take this out of context.
09:42:08 <kayabanerve[m]> (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)
09:42:21 <kayabanerve[m]> @sech1 No
09:42:32 <sech1> no wait, there's a second serialization function
09:42:35 <sech1> such a mess
09:42:54 <kayabanerve[m]> 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
09:42:58 <jeffro256[m]> ^^^^^^^
09:43:00 <kayabanerve[m]> But because images are in the message hash, which is serialized by do_serialize, they cannot
09:43:12 <kayabanerve[m]> I approved the PR on the premise images would be explicitly serialized by do_serialize
09:43:14 <kayabanerve[m]> They are not
09:43:19 <kayabanerve[m]> jeffro256: proved that with their test
09:43:47 <kayabanerve[m]> 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
09:44:00 <jeffro256[m]> Thanks for bringing this up on github btw kayabanerve[m] on making me reconsider and write that test lol
09:44:13 <jeffro256[m]> u were persistent good job 
09:44:25 <kayabanerve[m]> 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*
09:44:35 <kayabanerve[m]> Yet if it wasn't for the message indirection, that'd be an incorrect assumption
09:45:12 <kayabanerve[m]> 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
09:45:39 <kayabanerve[m]> 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
09:46:12 <kayabanerve[m]> I fully understand the distinct serialization functions. The issue is the distinct serialization function, meant to be complete, is not.
09:46:39 <sech1> "serialize(tx) || do_serialize(rct)" what does it mean?
09:46:52 <kayabanerve[m]> Also, to be clear, I commented on the original PR so all prior participants got pinged.
09:47:07 <kayabanerve[m]> sech1: You used hash(do_serialize(rct)) as a cache key
09:47:15 <kayabanerve[m]> I'm calling for hash(serialize(tx) concat do_serialize(rct))
09:47:38 <kayabanerve[m]> 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 
09:48:15 <sech1> no, I asked about "serialize(tx)"
09:48:18 <kayabanerve[m]> 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
09:48:23 <sech1> what serialize function specifically?
09:48:29 <kayabanerve[m]> ... convert the entire TX to bytes for the P2P layer?
09:48:35 <kayabanerve[m]> I believe it's just ::serialize
09:48:40 <kayabanerve[m]> I'm actually unsure.
09:49:08 <sech1> there is serialize_rctsig_base and serialize_rctsig_prunable
09:49:12 <kayabanerve[m]> ... 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
09:49:16 <sech1> I think both should be called
09:49:24 <kayabanerve[m]> sech1: I'm talking about  the entire TX
09:49:25 <kayabanerve[m]> Not just RCT
09:49:37 <kayabanerve[m]> This would make verifyRctCached take the entire TX, not just RCT
09:50:06 <kayabanerve[m]> 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
09:50:08 <sech1> ::serialize would just go through the same BEGIN_SERIALIZE/END_SERIALIZE macros that miss some field, right?
09:50:23 <kayabanerve[m]> Please see the concat
09:50:42 <kayabanerve[m]> It's serialize on the TX, missing the mix ring. It's do_serialize on the RCT, including the mix ring. That is complete.
09:50:53 <kayabanerve[m]> jeffro256: Will you update your current PR or make a followup PR to update the cached transcription?
09:51:18 <sech1> ahhhhh
09:51:18 <kayabanerve[m]> (to AT LEAST key images || do_ser(rct), but I've made my preference/what I'll sign off on clear)
09:51:20 <sech1> I'm slow today
09:51:28 <kayabanerve[m]> Or should I make the followup PR 
09:51:41 <sech1> so you want verRctNonSemanticsSimpleCached to have the second parameter - tx, and serialize it too?
09:51:41 <kayabanerve[m]> sech1: All good. I get to be stressed :p
09:51:44 <jeffro256[m]> Yeah I can write that 
09:51:59 <kayabanerve[m]> it doesn't need two since passing tx passes rct uinderneath it
09:52:10 <kayabanerve[m]> It just needs rct -> entire TX
09:52:16 <kayabanerve[m]> Thanks jeffro256. Let me know
09:56:46 <sech1>       ::do_serialize(ar, tx);
09:56:46 <sech1>       ::do_serialize(ar, tx.rct_signatures);
09:56:51 <sech1> kayabanerve[m] like this? ^^^^
09:57:26 <kayabanerve[m]> 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 :)
09:57:38 <kayabanerve[m]> So long as someone does. I can also just do it myself lol. It's not a big PR. One line add :p
09:58:09 <kayabanerve[m]> I just pointed to jeffro256 since they were already working on this with an open PR
09:58:30 <sech1> wait, there's ::do_serialize and ::serialize? wtf
09:59:05 <sech1> jeffro256[m] can you add it to your PR? To keep new tests and fixes together
09:59:09 <jeffro256[m]> yeah that was my reaction a couple weeks ago looking a cryptonote::transaction lol
09:59:22 <jeffro256[m]> Yeah of course 
09:59:24 <kayabanerve[m]> jeffro256: just rewrite it in rust already
09:59:32 <kayabanerve[m]> :p
09:59:35 <sech1> so the function should be changed to "bool verRctNonSemanticsSimpleCached(cryptonote::transaction& tx)"
09:59:50 <sech1> and then  ::do_serialize(ar, tx);  ::do_serialize(ar, tx.rct_signatures); in the code of the function
10:00:19 <kayabanerve[m]> With jeffro's test for mixRing inclusion in do_serialize (and the other pleasant goodies)
10:00:24 <kayabanerve[m]> That ensures there's nothing we miss
10:00:37 <kayabanerve[m]> Because just do_ser(rct) misses key images
10:00:46 <kayabanerve[m]> Serializing the entire TX won't, yet will have relative output indexing. Hence...
10:01:04 <sech1> output indexing?
10:01:09 <sech1> but this cache is for the mempool
10:01:49 <jeffro256[m]> I was gonna use cryptonote::get_transaction_hash since that *should* contain serialization for transaction prefix + blob cache And its usually cached well
10:02:21 <sech1> cryptonote::get_transaction_hash should be good enough too
10:02:25 <kayabanerve[m]> 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.
10:02:35 <jeffro256[m]> BUT PLEASE DOUBLE CHECK 
10:03:09 <kayabanerve[m]> Uhhh TX hash, if the e2e hash, should be prefix, base, and prunable
10:03:26 <kayabanerve[m]> Which would be everything
10:03:30 <jeffro256[m]> yes 
10:03:54 <jeffro256[m]> Sorry I meant get_transactio_hash in addition to do_serialize(rctSig)
10:03:54 <kayabanerve[m]> It uses the network serialization, not the signature serialization so... meets my goals 👍️
10:04:21 <kayabanerve[m]> I assumed you meant g_t_h instead of ser(tx) ;) All good
10:05:19 <sech1> so "hash(TX hash || do_serialize(rctSig))" will be used for caching, correct?
10:05:37 <kayabanerve[m]> Yes
10:05:51 <sech1> ok, now we're all on the same page
10:10:13 <sech1> TX hash as in what we use on the explorer, like in the URL here: https://xmrchain.net/tx/8d28f4d4601c5a0c2010446de1937127bce61af7d2c5668b1cd59dffc24f52a3 ? If it's this hash, then it really must have everything in it
10:10:26 <sech1> I think cryptonote::get_transaction_hash is this hash, correct me if I'm wrong
10:11:03 <sech1> "hash(TX hash || do_serialize(rctSig))" is good then
10:12:26 <sech1> 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
10:12:52 <kayabanerve[m]> 👍️
12:31:15 <tevador> there is a flooding attack on testnet
12:51:40 <sech1> No, it's one of p2pool users running a script to provide transactions
12:51:56 <sech1> We'll be running tests next week
15:18:38 <selsta> dEBRUYNE: p2pool will hardfork so target is beginning of February
15:25:21 <sech1> 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
15:37:47 <selsta> I meant target for putting out the new monero release
15:39:19 <sech1> jeffro256[m] kayabanerve[m] I'm testing this patch on my nodes now: https://paste.debian.net/hidden/1e016994/ - you can add it as is to your PR, or maybe you already have your own solution
16:30:11 -xmr-pr- ovhpse opened issue #8708: Exception: boost::wrapexcept<boost::system::system_error> when using T...
16:30:11 -xmr-pr- > https://github.com/monero-project/monero/issues/8708
16:38:54 <UkoeHB> yeah hashing the tx id is the right way to go
16:43:47 <llacqie[m]> 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?
16:45:18 <llacqie[m]> I use a binary compiled from the master branch with a back ping patch
16:47:57 <llacqie[m]> And I launch it with DNS_PUBLIC="tcp://8.8.8.8"
16:49:20 <llacqie[m]> I get this error about once a day
17:03:27 <selsta> llacqie[m]: I have seen this a couple times but usually don't get it once per day
17:15:04 <hyc> bad filedescriptor means somebody closed it without removing all references to it first
17:16:58 <hyc> probably a race condition with an async close
17:20:06 <hyc> that would most likely be a bug in libunbound
18:17:34 <dEBRUYNE> selsta: Thanks. As far as I read, it only concerns miners, not actual nodes, correct (re: the hard fork)?
18:18:03 <sech1> yes, only p2pool miners
19:39:53 <jeffro256[m]> sech1 kayabanerve patch was pushed + a couple unit tests