jeffro256[m]: can this be done in a separate PR later? monero-project/monero #8794#issuecomment-1484211279

Yes it can be, I'll put it in my unit test PR and make the unit test PR depend on #8794

It's important that #8794 is merged soon tho, certainly included in the next release

ok that's why i'm asking

i assume the unit test + the other change you are proposing aren't relevant for the release?

Nope they're just to hopefully prevent similar future bugs

not sure if you saw it but the tests CI failed on the unit test PR

Yes it fails for the master branch but passes with #8974 + my suggested change

Sorry that's probably confusing

oh ok

Lemme restructure it real quickl

no, it's fine if you wait for it to get merged and then force push to re-run tests

probably the easiest way

I'll push that suggested change tho with my PR

Does describe_transfer (getmonero.org/resources/developer-g…s/wallet-rpc.html#describe_transfer) verify that a partially-signed multisig tx's signatures are valid?

no

What do you mean no

I mean the opposite of yes

Maybe

<UkoeHB> "I mean the opposite of yes" <- I know I'm just goofing around for a bit lol

.merges

when tag and point release?

luigi1111w: ^^

done

Thanks! I'm updating my nodes now, will also make reproducible build in the evening.

Running v0.18.2.1 on my nodes now, 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.

Also, p2pool log now looks like this: paste.debian.net/hidden/ea0f04ca

So all transactions it downloads before adding a block, also get sent via ZMQ

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

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

it used to be stable 96 out/32 in

25 out/10 in now

they're dropping like flies :D

and now 68 out/12 in, hmmm

now 51 out/11 in

very weird

34 out/12 in

something's not right

will update and compare

both nodes show this behavior

Maybe it's counting the temp connections back to determine reachability ? That was borked and got fixed recently.

IIRC those weren't counted but who knows if they do now.

Do you mean my node is making these connections and counting them in "status" output?

I mean it might.

Just a guess. Something related changed recently, and could explain what you see.

Ok, I'll try to set out_peers to 1000 and wait until it gets enough real connections, and then drop back to 96

It didn't help

i see similar behaviour

there were barely any changes between v0.18.2.0 and v0.18.2.1

shouldn't be too difficult to figure out what causes this

moneromooo could this be the reason: paste.debian.net/hidden/2145fe90 ???

NFT transactions fail verification and my node then disconnects

This one has large tx_extra p2pool.io/explorer/tx/1ecce7898a4e1…66d6b511ee896c318b3445aea1cc926e40f

I guess we should wait with release :D

Well, if the logs says a connection is dropped due to tx verification failure, then yes, it's the reason for dropped connections :P

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

I guess it will be less of an issue as more and more people update

a hardfork lol

With some lube.. plowsof, im assuming your node likes seths?

And only drops bs connections?

huh, my node finally reached stable 96 out/17 in

lol, just as I said it, it dropped to 39 out

Sech***, sorry. Seth is the traitor.

it all depends on when the NFT guy sends a new batch of transactions :D

yes it becomes stable, then drops right back down

Time to update node... ^_^

So whichever nodes happen to propagate the NFT tx to you, they become non grata

Direct them to #monero-support:monero.social . Ill be waiting

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

it's better to fix this connection drop before release

agree that seems a bit too risky to deploy lol

I said "why don't you try on stagenet/testnet?" but "No"

I'll keep running v0.18.2.1 on my nodes

we did try

the problem is no one is spamming testnet with NFTs

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.

That's true

we can't just keep connection to a node that sends an invalid tx

xmrfn: With all due respect, that is foolish under the current conditions

Old nodes will eventually get the blocks. As long as miners are prodded to update, we should be good ^_^

so this tx_extra check shouldn't be a "verification failure", it should just silently drop it

We could make an exception for NFTs. Keep the connection

Should still allow broadcasting to those nodes

(Ideally)

And someone's making NFTs without IPFS ?

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.

0-conf is by definition accepting of a certain level of risk

0 conf with bloated txpool is dangerous too

The bigger problem with 0conf is if it doesnt propogate to mining nodes

I'll comment these two lines: github.com/monero-project/monero/bl…rc/cryptonote_core/tx_pool.cpp#L226 and run my nodes with this fix.

tx_memory_pool::add_tx already "ignores" transactions without declaring them "failed", in other parts of the code

So you're also commenting out tvc.m_tx_extra_too_big = true; ? Just checking

yes

will do the same (interrupts build)

If you do that, you'll almost certainly relay those txes.

m_tx_extra_too_big is not used anywhere in the code

See handle_notify_new_transactions, you probably want to check m_tx_extra_too_big there, not comment it out in txpool.

And not add it if set.

wait, so if m_verifivation_failed is not set, it will be relayed even if not added to mempool?

ok

But do test. I only read the code quickly :)

time to spam testnet i guess

that fix didn't help, it says "Tx verification failed, dropping connection" and drops connection ultimately because add_tx returns false

I'll add check for m_tx_extra_too_big into core::handle_incoming_txs to avoid connection drops

agree with this fix logic. Drop big extra txs but don't ban nodes

Running with this patch now: paste.debian.net/hidden/dcb83291 - so far so good

plowsof11 ^

yeah, it's not dropping connections anymore

this is the node that p2pool.io/explorer uses, so you can check in realtime if any large tx_extra leak into mempool

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...?

yes, dropping connection large tx_extra nodes is the best way when they are in minority, both by their hashrate and overall count

but as of now, risk of network split is too high when new nodes are minority

will try sech1 thanks , ive got a jillion outputs for testnet if needed 💢

My second node is running fine too now

I'll keep testing this patch

I'm building the same, will deploy when it completes

MMmmmmmm I love the smell of testing in production

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?

yes tagging .2 is better

it's possible to force push tags but isn'r recommended

I'll test my patch some more and will make a PR to both master and release-v0.18

I have a 3rd node at home, will test there too :D

My testnet morbs arent showing on xmrchain 💢

I lie. She's there

sech1 running patch on 1 node, connections building, no drops , will move to a 2nd

They're still showing in xmrchsins tx though - they shouldn't be (?)

Xmrchains explorer*

Txpool*

xmrchain probably didn't update

we need to confirm that sending an ordinal connected to a node with release+patch doesnt work ^^

will confirm later today, ive got diffs/wallets/morbs everywhere

Hi, can somebody break down the db-sync-mode parameters: safe, fast, fastest; sync, async; and which the default is?

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

not sure about sync / async

Can fastest be used with SSD?

there's no point with it

are any more changes expected to the v0.18.2.1 tag?

or am I reading correctly that a retag to v0.18.2.2 is forthcoming?

we will merge another patch and tag v0.18..2.2

ok

opened a prepare v0.18.2.2 patch monero-project/monero #8805

I've created PR 8806 and 8807 with the tx_extra fix

is there any scenario now where previously a node would drop connection that now keeps connection?

just want to make sure we don't increase any attack vector

with before i mean v0.18.2.0

this line explicitly checks the condition before changing previous behavior

so it will only change for transactions that trigger m_tx_extra_too_big

sure, a node can send an otherwise invalid transaction that just happens to be filtered by tx_extra filter first

then it will stay connected

but the tx_extra filter will keep it out of mempool anyway, so it will be harmless

and it will never be mined if it's invalid somewhere else

would that allow for keep spamming the same tx and the node wastes time verifying it again and again? or similar attacks?

theoretically, yes

you can spam a transaction with big tx_extra

without the fix, you will get disconnected

but you can still connect again and spam

not much difference

If you get kicked enough, you get banned.

Enough times I mean.

I didn't see my node banning other peers

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.

It seems "Tx verification failed, dropping connection" drops connections with add_fail = false: drop_connection(context, false, false);

so you won't get banned just from failed tx verifications

exactly

not sure if that was an oversight or intentional

which means my patch doesn't change this particular attack vector

if it's even an attack vector

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.

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.

selsta I checked, and tx_extra check is done before all CPU intensive checks, so spamming it will not give any noticeable effect

I think tevador put it before CPU intensive checks on purpose

jeffro256[m] UkoeHB are you also ok with the patch now?

Don't know yet, still looking at it

Personally, I can tolerate longer block propagation times for non-conforming blocks, all I really care about is the overly aggressive connection dropping

Longer block propogation time is a soft price that miners would have to pay to mine non conforming blocks

this PR fixes connection dropping

I'm running it on my nodes now

including the node for p2pool.io/explorer

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

Would help the "spaghetti"-ness that Ukoe mentioned

I'll functional test it realsoon

sech1: the patch introduces a critical bug, I'd fix asap

"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."

this?

This is how it works, it's not a bug

even before my PR

yes, you are short-circuiting the error checks

"YOU"?

It's not my code

It is what it is

stop bullshitting me

this is your bug: if (!tvc[i].m_tx_extra_too_big) ok = false;

ok needs to be false if any error checks after the tx extra too big check are false

so you're suggesting to run all other checks too, even if tx_extra check failed?

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

making it vulnerable to spamming CPU-heavy checks?

Do you know the concept of "early outing" in programming?

You're telling me to rewrite the logic, I'm telling you no

That appears to be inherently needed to distinguish between otherwise fine txs with bad tx_extra and all around bad txs, no?

I wrote a detailed breakdown here: monero-project/monero #8806#discussion_r1149597742

I know exactly how this code will work

and I don't consider it a bug

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

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

Because there are concensus checks after the tx_extra check

jeffro256[m] read monero-project/monero #8806#discussion_r1149597742 please

These checks are not even consensus

it's txpool checks

I'm not going to change my PR's logic

Do your own PR, or show me an _actual_ critical bug in it

I know you don't like "spaghetti" code, neither do I, but this is not refactoring!

it's a bugfix, goddamit

actual critical bug = at least show where monero-project/monero #8806#discussion_r1149597742 is wrong

Fuck, I'm the only one trying to fix broken things here while others are obsessed with refactoring

Logging out for 24 hours, until you guys figure it out

Does monero-project/monero #8808 check out theoretically? Would also address fee too low issue that @UkoeHB mentioned

Have yet to test it

Also this technique can be easily applied to other future mempool protections

jeffro256[m]: afaict that would work

there are several other locations where it seems to drop connection if handle_incoming_tx() fails though

Yes, I was just looking at those, like fluffy blocks

Or regular new block notifies

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?

I just don't want to create a scenario in which the Blockchain waits on the mempool forever

theoretically they can ignore the mempool, idk about the implementation

Looking at the protocol handler code makes me want to cry. Just found logic nested 10x

fwiw sech1 is correct. don't conflate bug fixing with refactoring.

Agreed

@hyc would you mind looking at #8808 ? Would also fix peer dropping due to fees that are too low

ok, will check it out later