01:49:58 <selsta> jeffro256[m]: can this be done in a separate PR later? https://github.com/monero-project/monero/pull/8794#issuecomment-1484211279
01:51:01 <jeffro256[m]> Yes it can be, I'll put it in my unit test PR and make the unit test PR depend on #8794 
01:51:58 <jeffro256[m]> It's important that #8794 is merged soon tho, certainly included in the next release 
01:52:07 <selsta> ok that's why i'm asking
01:52:39 <selsta> i assume the unit test + the other change you are proposing aren't relevant for the release?
01:53:05 <jeffro256[m]> Nope they're just to hopefully prevent similar future bugs 
01:53:27 <selsta> not sure if you saw it but the tests CI failed on the unit test PR
01:54:07 <jeffro256[m]> Yes it fails for the master branch but passes with #8974 + my suggested change 
01:54:18 <jeffro256[m]> Sorry that's probably confusing 
01:54:22 <selsta> oh ok
01:54:29 <jeffro256[m]> Lemme restructure it real quickl
01:54:55 <selsta> no, it's fine if you wait for it to get merged and then force push to re-run tests
01:55:00 <selsta> probably the easiest way
01:59:00 <jeffro256[m]> I'll push that suggested change tho with my PR 
02:06:52 <Alex|LocalMonero> Does describe_transfer (https://www.getmonero.org/resources/developer-guides/wallet-rpc.html#describe_transfer) verify that a partially-signed multisig tx's signatures are valid?
02:29:48 <UkoeHB> no
02:32:15 <xmrlover[m]> What do you mean no 
02:50:18 <UkoeHB> I mean the opposite of yes
02:53:53 <DanIsnotthemanBr> Maybe
04:11:46 <xmrlover[m]> <UkoeHB> "I mean the opposite of yes" <- I know I'm just goofing around for a bit lol 
12:56:51 <sech1> .merges
12:56:51 -xmr-pr- 8794 8801
13:01:21 <sech1> when tag and point release?
13:06:54 <selsta> luigi1111w: ^^
13:19:33 <luigi1111w> done
13:25:02 <sech1> Thanks! I'm updating my nodes now, will also make reproducible build in the evening.
14:06:32 <sech1> Running v0.18.2.1 on my nodes now, https://p2pool.io/explorer/ also update. I noticed one small issue (not really an issue, just the way it works now): since transactions with large tx_extra are not added to mempool now, monerod has to download them when it receives a new block with these transactions, which slows down block propagation a bit.
14:06:32 <sech1> Also, p2pool log now looks like this: https://paste.debian.net/hidden/ea0f04ca/
14:07:06 <sech1> So all transactions it downloads before adding a block, also get sent via ZMQ
14:07:42 <sech1> Which is technically correct, but do we want it? If a transaction doesn't pass mempool filters, it probably shouldn't be sent via ZMQ
14:12:18 <sech1> Also my nodes struggle with getting connections: Height: 2851199/2851199 (100.0%) on mainnet, not mining, net hash 2.50 GH/s, v16, 35(out)+9(in) connections, uptime 0d 0h 31m 34s
14:12:39 <sech1> it used to be stable 96 out/32 in
14:13:13 <sech1> 25 out/10 in now
14:13:18 <sech1> they're dropping like flies :D
14:13:36 <sech1> and now 68 out/12 in, hmmm
14:13:45 <sech1> now 51 out/11 in
14:13:48 <sech1> very weird
14:14:04 <sech1> 34 out/12 in
14:14:10 <sech1> something's not right
14:14:36 <plowsof11> will update and compare
14:14:45 <sech1> both nodes show this behavior
14:15:02 <moneromooo> Maybe it's counting the temp connections back to determine reachability ? That was borked and got fixed recently.
14:15:21 <moneromooo> IIRC those weren't counted but who knows if they do now.
14:16:16 <sech1> Do you mean my node is making these connections and counting them in "status" output?
14:16:30 <moneromooo> I mean it might.
14:17:03 <moneromooo> Just a guess. Something related changed recently, and could explain what you see.
14:17:04 <sech1> Ok, I'll try to set out_peers to 1000 and wait until it gets enough real connections, and then drop back to 96
14:19:14 <sech1> It didn't help
14:20:53 <plowsof11> i see similar behaviour 
14:21:04 <selsta> there were barely any changes between v0.18.2.0 and v0.18.2.1
14:21:14 <selsta> shouldn't be too difficult to figure out what causes this
14:21:17 <sech1> moneromooo could this be the reason: https://paste.debian.net/hidden/2145fe90/ ???
14:21:37 <sech1> NFT transactions fail verification and my node then disconnects
14:22:10 <sech1> This one has large tx_extra https://www.p2pool.io/explorer/tx/1ecce7898a4e1c7334241c834ba4866d6b511ee896c318b3445aea1cc926e40f
14:22:38 <sech1> I guess we should wait with release :D
14:23:00 <moneromooo> Well, if the logs says a connection is dropped due to tx verification failure, then yes, it's the reason for dropped connections :P
14:23:36 <sech1> so not only it doesn't like big tx_extra in mempool now, it also doesn't like whoever sent it, i.e. v0.18.2.0 nodes :D
14:23:53 <sech1> I guess it will be less of an issue as more and more people update
14:24:01 <plowsof11> a hardfork lol
14:24:30 <ofrnxmr[m]> With some lube.. plowsof, im assuming your node likes seths?
14:24:40 <ofrnxmr[m]> And only drops bs connections? 
14:24:43 <sech1> huh, my node finally reached stable 96 out/17 in
14:24:55 <sech1> lol, just as I said it, it dropped to 39 out
14:25:01 <ofrnxmr[m]> Sech***, sorry. Seth is the traitor.
14:25:08 <sech1> it all depends on when the NFT guy sends a new batch of transactions :D
14:25:13 <plowsof11> yes it becomes stable, then drops right back down
14:26:03 <moneromooo> Time to update node... ^_^
14:26:11 <xmrfn[m]> So whichever nodes happen to propagate the NFT tx to you, they become non grata
14:26:36 <ofrnxmr[m]> Direct them to #monero-support:monero.social . Ill be waiting 
14:26:39 <sech1> hmm, it can potentially end up in network split: v0.18.2.1 nodes settle with only connections between each other, and v0.18.2.0 nodes will only connect to each other
14:26:51 <sech1> it's better to fix this connection drop before release
14:27:25 <selsta> agree that seems a bit too risky to deploy lol
14:27:45 <Rucknium[m]> I said "why don't you try on stagenet/testnet?" but "No"
14:27:53 <sech1> I'll keep running v0.18.2.1 on my nodes 
14:28:00 <sech1> we did try
14:28:13 <sech1> the problem is no one is spamming testnet with NFTs
14:28:55 <xmrfn[m]> Some thought needs to be given to the "fix" of how/why we drop. Surely dropping connection to nodes that send bad tx is a good policy.
14:29:18 <sech1> That's true
14:29:29 <sech1> we can't just keep connection to a node that sends an invalid tx
14:29:43 <Rucknium[m]> xmrfn: With all due respect, that is foolish under the current conditions
14:30:01 <moneromooo> Old nodes will eventually get the blocks. As long as miners are prodded to update, we should be good ^_^
14:30:04 <sech1> so this tx_extra check shouldn't be a "verification failure", it should just silently drop it
14:30:04 <xmrfn[m]> We could make an exception for NFTs. Keep the connection
14:30:16 <ofrnxmr[m]> Should still allow broadcasting to those nodes
14:30:34 <ofrnxmr[m]> (Ideally) 
14:30:55 <moneromooo> And someone's making NFTs without IPFS ?
14:32:13 <Rucknium[m]> The tx_extra limitation will also make 0-conf less safe by the way. For 0-conf safety you need consistent mempool policies across the network.
14:32:45 <xmrfn[m]> 0-conf is by definition accepting of a certain level of risk
14:32:45 <ofrnxmr[m]> 0 conf with bloated txpool is dangerous too
14:33:57 <ofrnxmr[m]> The bigger problem with 0conf is if it doesnt propogate to mining nodes 
14:34:03 <sech1> I'll comment these two lines: https://github.com/monero-project/monero/blob/release-v0.18/src/cryptonote_core/tx_pool.cpp#L226 and run my nodes with this fix.
14:34:38 <sech1> tx_memory_pool::add_tx already "ignores" transactions without declaring them "failed", in other parts of the code
14:35:57 <xmrfn[m]> So you're also commenting out  tvc.m_tx_extra_too_big = true; ? Just checking
14:36:58 <sech1> yes
14:37:23 <xmrfn[m]> will do the same (interrupts build)
14:37:23 <moneromooo> If you do that, you'll almost certainly relay those txes.
14:38:39 <sech1> m_tx_extra_too_big is not used anywhere in the code
14:38:43 <moneromooo> See handle_notify_new_transactions, you probably want to check m_tx_extra_too_big there, not comment it out in txpool.
14:38:54 <moneromooo> And not add it if set.
14:39:01 <sech1> wait, so if m_verifivation_failed is not set, it will be relayed even if not added to mempool?
14:39:09 <sech1> ok
14:39:32 <moneromooo> But do test. I only read the code quickly :)
14:39:39 <plowsof11> time to spam testnet i guess 
14:45:48 <sech1> that fix didn't help, it says "Tx verification failed, dropping connection" and drops connection ultimately because add_tx returns false
14:48:14 <sech1> I'll add check for m_tx_extra_too_big into core::handle_incoming_txs to avoid connection drops
14:52:17 <luigi1111w> agree with this fix logic. Drop big extra txs but don't ban nodes
14:53:20 <sech1> Running with this patch now: https://paste.debian.net/hidden/dcb83291/ - so far so good
14:53:31 <sech1> plowsof11 ^
14:54:12 <sech1> yeah, it's not dropping connections anymore
14:55:16 <sech1> this is the node that https://p2pool.io/explorer/ uses, so you can check in realtime if any large tx_extra leak into mempool
14:56:24 <xmrfn[m]> Maybe after v0.18.2.2 (v0.18.2.1 + this patch) becomes consensus, then go back to dropping nodes that propagate large tx? I guess this is a model for deploying similar tx acceptability checks in the future...?
14:57:09 <sech1> yes, dropping connection large tx_extra nodes is the best way when they are in minority, both by their hashrate and overall count
14:57:40 <sech1> but as of now, risk of network split is too high when new nodes are minority
14:59:25 <plowsof11> will try sech1 thanks , ive got a jillion outputs for testnet if needed 💢
15:02:11 <sech1> My second node is running fine too now
15:02:15 <sech1> I'll keep testing this patch
15:06:45 <xmrfn[m]> I'm building the same, will deploy when it completes 
15:11:14 <xmrfn[m]> MMmmmmmm I love the smell of testing in production
15:12:20 <sech1> selsta IIRC it's impossible to change the tag once some commit has been tagget, right? So we'll have to tag v0.18.2.2?
15:12:39 <selsta> yes tagging .2 is better
15:12:54 <selsta> it's possible to force push tags but isn'r recommended
15:13:02 <sech1> I'll test my patch some more and will make a PR to both master and release-v0.18
15:13:19 <sech1> I have a 3rd node at home, will test there too :D
15:13:47 <ofrnxmr[m]> My testnet morbs arent showing on xmrchain 💢
15:14:52 <ofrnxmr[m]> I lie. She's there
15:14:53 <ofrnxmr[m]> https://testnet.xmrchain.net
15:18:10 <plowsof11> sech1 running patch on 1 node, connections building, no drops , will move to a 2nd
15:22:16 <ofrnxmr[m]> They're still showing in xmrchsins tx though - they shouldn't be (?)
15:24:48 <ofrnxmr[m]> Xmrchains explorer*
15:25:10 <ofrnxmr[m]> Txpool*
15:33:52 <sech1> xmrchain probably didn't update
15:43:03 <plowsof11> we need to confirm that sending an ordinal connected to a node with release+patch doesnt work ^^ 
15:46:38 <plowsof11> will confirm later today, ive got diffs/wallets/morbs everywhere 
16:20:31 <AbdullahKhan[m]> Hi, can somebody break down the db-sync-mode parameters: safe, fast, fastest; sync, async; and which the default is? 
16:27:18 <selsta> fastest is only useful if you store the blockchain in RAM, safe helps if you system often crashes or your external HDD might unplug during sync, fast is the default
16:27:36 <selsta> not sure about sync / async
16:28:58 <AbdullahKhan[m]> Can fastest be used with SSD?
16:31:33 <selsta> there's no point with it
16:49:53 <woodser[m]> are any more changes expected to the v0.18.2.1 tag?
16:51:13 <woodser[m]> or am I reading correctly that a retag to v0.18.2.2 is forthcoming?
16:51:22 <selsta> we will merge another patch and tag v0.18..2.2
16:52:09 <woodser[m]> ok
16:53:28 <selsta> opened a prepare v0.18.2.2 patch https://github.com/monero-project/monero/pull/8805
17:32:47 <sech1> I've created PR 8806 and 8807 with the tx_extra fix
17:36:53 <selsta> is there any scenario now where previously a node would drop connection that now keeps connection?
17:37:00 <selsta> just want to make sure we don't increase any attack vector
17:37:15 <selsta> with before i mean v0.18.2.0
17:37:46 <sech1> https://github.com/monero-project/monero/pull/8806/files#diff-1fb58b51f281d178c1a564bccf94bc813261a1fd585a9372e3d53999a25f9a53R1098
17:38:02 <sech1> this line explicitly checks the condition before changing previous behavior
17:38:25 <sech1> so it will only change for transactions that trigger m_tx_extra_too_big
17:38:50 <sech1> sure, a node can send an otherwise invalid transaction that just happens to be filtered by tx_extra filter first
17:38:56 <sech1> then it will stay connected
17:39:37 <sech1> but the tx_extra filter will keep it out of mempool anyway, so it will be harmless
17:40:39 <sech1> and it will never be mined if it's invalid somewhere else
17:48:51 <selsta> would that allow for keep spamming the same tx and the node wastes time verifying it again and again? or similar attacks?
17:50:32 <sech1> theoretically, yes
17:50:40 <sech1> you can spam a transaction with big tx_extra
17:50:51 <sech1> without the fix, you will get disconnected
17:51:00 <sech1> but you can still connect again and spam
17:51:07 <sech1> not much difference
17:51:28 <moneromooo> If you get kicked enough, you get banned.
17:51:38 <moneromooo> Enough times I mean.
17:51:44 <sech1> I didn't see my node banning other peers
17:52:31 <moneromooo> Maybe you did not wait enough, maybe those are silent, maybe there is a bug, maybe this particular reason does not use the scoring system.
17:53:45 <selsta> It seems "Tx verification failed, dropping connection" drops connections with add_fail = false: drop_connection(context, false, false);
17:53:57 <selsta> so you won't get banned just from failed tx verifications
17:54:00 <sech1> exactly
17:54:09 <selsta> not sure if that was an oversight or intentional
17:54:16 <sech1> which means my patch doesn't change this particular attack vector
17:54:45 <sech1> if it's even an attack vector
18:12:54 <Alex|LocalMonero> How does one verify someone else's partially-signed multisig tx signature without broadcasting it to the network with submit_multisig? The /sendrawtransaction endpoint of monerod doesn't accept multisig tx hexes, returns a sanity check failed.
18:14:22 <Alex|LocalMonero> If it did, one could complete the signatures and submit it to monerod's /sendrawtransaction with do_not_relay:true to get it checked without broadcasting it to the network.
18:58:13 <sech1> selsta I checked, and tx_extra check is done before all CPU intensive checks, so spamming it will not give any noticeable effect
18:58:41 <sech1> I think tevador put it before CPU intensive checks on purpose
19:02:14 <selsta> jeffro256[m] UkoeHB are you also ok with the patch now?
19:31:30 <jeffro256[m]> Don't know yet, still looking at it 
19:32:53 <jeffro256[m]> Personally, I can tolerate longer block propagation times for non-conforming blocks, all I really care about is the overly aggressive connection dropping 
19:33:37 <jeffro256[m]> Longer block propogation time is a soft price that miners would have to pay to mine non conforming blocks 
19:34:44 <sech1> this PR fixes connection dropping
19:35:00 <sech1> I'm running it on my nodes now
19:35:16 <sech1> including the node for https://p2pool.io/explorer/
19:37:24 <jeffro256[m]> Yes, it looks like it should work. This could be a good opportunity to add to the verification struct a category of "this is a valid tx to be included in a block but don't publish or broadcast this tx" more generlly 
19:38:32 <jeffro256[m]> Would help the "spaghetti"-ness that Ukoe mentioned 
19:39:07 <jeffro256[m]> I'll functional test it realsoon
20:04:12 <UkoeHB> sech1: the patch introduces a critical bug, I'd fix asap
20:07:38 <sech1> "So you can implicitly have a whole slew of things wrong with your tx in addition to the tx extra being too big, but the tvc won't record them."
20:07:40 <sech1> this?
20:07:53 <sech1> This is how it works, it's not a bug
20:08:03 <sech1> even before my PR
20:08:05 <UkoeHB> yes, you are short-circuiting the error checks
20:08:13 <sech1> "YOU"?
20:08:16 <sech1> It's not my code
20:08:21 <sech1> It is what it is
20:08:34 <sech1> stop bullshitting me
20:08:36 <UkoeHB> this is your bug: if (!tvc[i].m_tx_extra_too_big) ok = false;
20:09:18 <UkoeHB> ok needs to be false if any error checks after the tx extra too big check are false
20:09:49 <sech1> so you're suggesting to run all other checks too, even if tx_extra check failed?
20:09:55 <jeffro256[m]> Transaction with (e.g.) semantics error  and tx_extra too big will have the verification context set tx_extra_to_big to true but nothing else 
20:10:07 <sech1> making it vulnerable to spamming CPU-heavy checks?
20:10:25 <sech1> Do you know the concept of "early outing" in programming?
20:10:35 <sech1> You're telling me to rewrite the logic, I'm telling you no
20:11:47 <jeffro256[m]> That appears to be inherently needed to distinguish between otherwise fine txs with bad tx_extra and all around bad txs, no?
20:12:11 <sech1> I wrote a detailed breakdown here: https://github.com/monero-project/monero/pull/8806#discussion_r1149597742
20:12:20 <sech1> I know exactly how this code will work
20:12:24 <sech1> and I don't consider it a bug
20:15:09 <jeffro256[m]> There's a third option: tx_extra check fails, add_new_tx returns with tx_extra_too_big set, but there is another issue with the transaction, but ok is still set to true
20:15:34 <UkoeHB> ok looked deeper, it doesn't appear to be a bug but it does change the semantics of `handle_incoming_tx()` because it can now return true even when a tx wasn't added, which is potentially a bug
20:15:52 <jeffro256[m]> Because there are concensus checks after the tx_extra check
20:16:28 <sech1> jeffro256[m] read https://github.com/monero-project/monero/pull/8806#discussion_r1149597742 please
20:17:11 <sech1> These checks are not even consensus
20:17:14 <sech1> it's txpool checks
20:17:43 <sech1> I'm not going to change my PR's logic
20:17:54 <sech1> Do your own PR, or show me an _actual_ critical bug in it
20:18:26 <sech1> I know you don't like "spaghetti" code, neither do I, but this is not refactoring!
20:18:33 <sech1> it's a bugfix, goddamit
20:19:48 <sech1> actual critical bug = at least show where https://github.com/monero-project/monero/pull/8806#discussion_r1149597742 is wrong
20:20:47 <sech1> Fuck, I'm the only one trying to fix broken things here while others are obsessed with refactoring
20:21:06 <sech1> Logging out for 24 hours, until you guys figure it out
21:35:11 <jeffro256[m]> Does https://github.com/monero-project/monero/pull/8808 check out theoretically? Would also address fee too low issue that @UkoeHB mentioned 
21:35:17 <jeffro256[m]> Have yet to test it 
21:40:36 <jeffro256[m]> Also this technique can be easily applied to other future mempool protections 
21:48:07 <UkoeHB> jeffro256[m]: afaict that would work
21:50:13 <UkoeHB> there are several other locations where it seems to drop connection if handle_incoming_tx() fails though
21:55:56 <jeffro256[m]> Yes, I was just looking at those, like fluffy blocks
21:55:57 <jeffro256[m]> Or regular new block notifies
22:03:13 <jeffro256[m]> At a high level view, transactions in blocks to be verified do not need to always pass through the mempool to get to blockchain, right? Sometimes the Blockchain can request blocks directly from other peers, yeah?
22:03:29 <jeffro256[m]> I just don't want to create a scenario in which the Blockchain waits on the mempool forever 
22:06:40 <UkoeHB> theoretically they can ignore the mempool, idk about the implementation
22:07:47 <jeffro256[m]> Looking at the protocol handler code makes me want to cry. Just found logic nested 10x 
22:08:18 <hyc> fwiw sech1 is correct. don't conflate bug fixing with refactoring.
22:08:57 <jeffro256[m]> Agreed 
22:11:22 <jeffro256[m]> @hyc would you mind looking at #8808 ? Would also fix peer dropping due to fees that are too low 
22:14:10 <hyc> ok, will check it out later