Hi guys, as far as I know, xmr->btc atomic swaps cannot be implemented correctly until Monero adds transaction pre-signing (read this in a commit blog post). Does anyone know if this will be added in the next hard fork?

This is the blog post: comit.network/blog/2021/07/02/transaction-presigning

nantuk[m]: for transaction chaining (needed for xmr->X swaps) Monero would need to fork to something like seraphis (WIP next-gen tx protocol), which is at least 2 years away probably.

<UkoeHB> "nantuk: for transaction chaining..." <- Oh all right, thanks

get_fee_quantization_mask is static and will be until we have a major fee overhaul, so I can ignore it as an RPC value and instead write it as a constant myself... right?

The only other consideration that may be relevant is if the unit fees UkoeHB proposed would be automatically applicable by wallets if they do operate with the RPC returned mask properly (setting the mask to 1 and the fee per kb to 1/2/... as well).

jberman: thanks for taking on 7760 rewrite as node performance is greatly improved on it. will it make it into v0.18 or is there not enough time to get it in?

r4v3r23: Not necessarily working on a rewrite, just working through understanding it in full to review it. I think I'll be done in the next couple days but possibly not

<kayabaNerve> "get_fee_quantization_mask is..." <- It should be but note the upcoming fork modifies the fee algo, which you're going to want to make the RPC call to get_fee_estimate for, which returns the quantization mask anyway

hyc I think we have a problem with lmdb in the latest master

I'm debugging broken solo mining in xmrig when ZMQ is enabled

this is what I get on monerod side: paste.debian.net/hidden/d3297b7c

so the blockchain height changes to 2644329 at 18:11:56.104 because new block was added

but then it starts changing between 28 and 29 erratically

xmrig calls get_block_template RPC as soon as it gets new block ZMQ notification, and get_block_template returns 2644328 instead of 2644329

I did a workaround in xmrig: xmrig/xmrig #3067/commits/db9069897df3e9ac750d57d501771f6200f8ff41

but the root cause of what's happening is beyond my understanding

and this is 100% reproducible. Every single new block - the same behavior

and it doesn't happen in release-v0.17 ?

release-v0.17 works fine

how long does it take to reproduce the issue?

100% reproducible

bisect.. again :D

I just start mining solo xmrig with ZMQ enabled

and wait for next block

and boom!

I can bisect, but maybe hyc can recognize this behavior?

there is a different issue on reorgs, maybe they are related

It's not the same thread, so each thread will have a different view of hte db until the txn commit. Did you account for this ?

"release-v0.17 works fine" suggests yes, but... just making sure :)

I didn't account for this

so calling get_block_template immediately after is not safe?

It depends after what.

after handle_block_to_main_chain

If calling on hte same thread, it'll be fine.

so handle_block_to_main_chain sends miner notification to xmrig, xmrig calls get_block_template RPC immediately

and this RPC accesses db on another thread, right?

so it's not safe?

If handle_block_to_main_chain hasn't commited the db txn yet, it's not safe.

But a process creation etc is slow, so...

If the miner notification is synchronous, it's definitely not safe. If async, it's not safe, but probably ok in practice.

So not OK :D

what process creation? It's ZMQ pub send

This is probably an existing monero bug I think...

so a few bytes over TCP connection

Oh. I'm not familiar with those. I'm familiar with the execve one.

it's sent in the end of handle_block_to_main_chain, already after m_db->add_block() succeeded

So I'm guessing zmq exacerbates then.

And I'm guessing there's a dtor that commits at scope end ?

Hmm. Seems unlikely, IIRC commits are explicits... Anyway, worth checking.

I'm bisecting now (going one month back at a time, recompiling and testing)

if you start with master~300 you will have to do 8 compiles

did it yesterday to find the glibc commit

9 at most... Sorry for pedantic, but... :D

I think. Maybe. Off by ones are sooooo programmerese...

hmm yes 9, 8 if you start with master~255

commit 0f7b20a1ce5510ea730ce3ec6454ef4822309a51 (end of March) works

I'll keep looking

monero-project/monero #7937 - this is the only one directly related to src/rpc or src/cryptonote_core

sech1: works as in doesn't have the issue?

yes

sech1: not sure if you have ooo on ignore but it might be monero-project/monero #7937

it would fit date wise

I'm already bisecting around the date it was merged

<r4v3r23[m]> "jberman: thanks for taking on 77..." <- there are examples when less isolated changes merged without proper review and the main benefit of 7759/7760 isn't node performance (I didn't even measure speed of anything) and it must be included even before v0.18 and v0.18 has bigger troubles except broken connection code, not sure how explicit request for reviews will speed up it;

<ooo123ooo1234567> "there are examples when less..." <- all i know is that nodes running 7760 are much more stable

r4v3r23[m]: unfortunately this level of understanding doesn't reflect all benefits from that patch

im just a user, i dont need to understand. ever since using selsta's node my connections have been rock solid

it's like you're approaching some fundamental improvements and someone judging it by cover (code style), it only emphasize level of understanding of those who are reviewing

<jberman[m]> "It should be but note the..." <- Mind elaborating? I wasn't aware of this.

My issue with get_fee_estimate is it's not reproducible.

My issue with the quantization mask is no one else on the planet uses it so I either need to craft custom code handling OR isolate it in Monero, which I know I can do so long as it's static.

So if I'm already writing my own fee algorithm implementation, and the mask is static... that's when the question is raised of why am I calling g_f_e

kayabanerve[m]: And this is a non-starter which can't be worked around without patches to monerod :/

jberman[m]: Thanks

<r4v3r23[m]> "im just a user, i dont need to..." <- it's supposed to be used on every node, every time you're falling back to centralized solutions it harms in long term

selsta yes, monero-project/monero 83bb027 is where the behavior changed

previous commit works, this one doesn't

or I'd rather say both work, it's just xmrig that needs to change how it calls get_block_template

sech1: wow

sech1: indeed, it's a try to justify whatever is needed to be justified, the same as with unbound: previously ubuntu 16 is sad, after ubnuntu 16 is 1 year after eol

sech1: should we undo 83bb027?

of course not

btw p2pool isn't affected by this because it uses miner notifications sent from the same thread

i mean if it's buggy behavior on monerod then reverting is better than a workaround on xmrig. if it was just an implementation issue with xmrig then it's fine.

I don't think it's buggy

get_block_template maybe should be fixed to wait for db lock before proceeding

I already have a workaround for xmrig

jberman[m] should look into it, I'm not familiar with the db code

it's not buggy per se - get_block_template returns old block template if called before db is ready with the new data. So it's correct from RPC point of view

but adding a lock there would improve it

Yes, best to set a flag instead of calling, and call in cleanup_add_blocks if the flag is set.

There's already a similar flag, forgot what I added it for, but similar thing.

<sech1> "but the root cause of what's..." <- How would you describe the root cause ?

cleanup_add_blocks? I can't find it

<sech1> "but adding a lock there would..." <- or not improve, what is the root cause ?

ooo123ooo1234567 you're on my ignore list. I just checked libera logs and I see I saved a lot of time by not seeing your comments. 99% garbage as usual

cleanup_handle_incoming_blocks

I'm reasonably sure this gets called for locally found blocks. Double check though.

sech1: There is a fix for ubuntu 16 compatibility , and comment with link to patch related to db, all are garbage ?

you're just following crowd, it isn't about garbage or not garbage and don't want to admit it

yes, it's for embedded miner

core::handle_incoming_block doesn't call it

Right. It's called after it.

prepare/handle/cleanup.

If succesful. cleanup isn't called on error.

1st question is how blackbox patch with lmdb locks in high layer of rpc changed something for rpc calls - there is certainly some overlook, 2nd question - are there any benefits from lmdb locks in rpc, it's certainly not long term solution for race conditions

"1st question is how blackbox patch with lmdb locks in high layer of rpc changed something for rpc calls - there is certainly some overlook, 2nd question - are there any benefits from lmdb locks in rpc, it's certainly not long term solution for race conditions"

-ooo123

zmq notification within src/cryptonote_core/blockchain.cpp is done before commit of tx into db, it's possible to reorder it, but why rpc patch with lmdb locks added some inconsistency if it was supposed to improve something

"zmq notification within src/cryptonote_core/blockchain.cpp is done before commit of tx into db, it's possible to reorder it, but why rpc patch with lmdb locks added some inconsistency if it was supposed to improve something"

--ooo123

ooo123 clearly didn't read the code

sech1: if you is going to read code before submitting the next PR then I would be very happy

s/is/are/

sech1: as soon as someone will submit patch it will be possible to test: correct patch can't be written without code reading with high probability

get_block_template holds m_tx_pool lock, and this lock is released in cleanup_handle_incoming_blocks, so all should be good. Need to wait for jberman[m] to clarify what is happening there

<sech1> "ooo123ooo1234567 you're on my..." <- libera logs doesn't contain referenced msg in reply (it's visible only on matrix side in the worst case)

*Blockchain::create_block_template holds m_tx_pool lock

<sech1> "get_block_template holds..." <- read code, it's the best clarification tool

<sech1> "get_block_template holds..." <- I've found the root cause

indeed, it's due to those low level lmdb locks added into high level rpc

* level lmdb "locks, * locks" added

<ooo123ooo1234567> "1st question is how blackbox..." <- 1st question answered, 2nd question - it's more harm than benefits since db should be fixed from bottom to top without adding intrusive db "locks" into high layer blindly

<ooo123ooo1234567> "zmq notification within src/..." <- reordering will not help, it's clear now why rpc patch mad it something worse

s/mad/made/

s/mad/made/, s/it//

how is it garbage if these question helped with understanding of the problem ?

"db should be fixed from bottom to top" -> usual garbage comment from ooo123, this is why you're on ignore

I've found the root cause too

db_rtxn_guard is locked in before the new data is fully commited to the db

no need to rewrite db, and the db guard is correct there

before, it would be executing in parallel with db update

<sech1> ""db should be fixed from..." <- it's impossible to use low level locks in high layer with encapsulated objects, in the worst case it could be deadlock / race condition / consistency

the sequence of events uncovered with printf-debugging: paste.debian.net/hidden/f3e2f195

<sech1> "no need to rewrite db, and the..." <- it's possible that db guard will be there in final version, but only after all layers below are fixed, not before

sech1: I've just read code to understand it

so it's correct that get_block_template returns old data and the db guard prevented returning possibly incosistent data

sech1: consistency is expected across all api from daemon, it isn't consistent to see different from zmq and rpc

events may be asynchronous (it's needed for speed), but they should be consistent in order to be reliable for external user

this can be fixed by moving db_rtxn_guard after m_tx_pool.lock(); inside create_block_template

trying it now

it would certainly be bad practice to take a db lock long before it's needed

sech1: it means move db_rtxn_guard (added blindly in 7937) from core_rpc_server::on_getblocktemplate into Blockchain::create_block_template

yes, it takes the db lock and then waits for m_tx_pool.lock() for a long time

yeap, moving db_rtxn_guard to the right place works like a charm

PR soon

sech1: wow, so once you've found the root cause it was possible to fix it; so unexpected

sech1: 7937 is still good ? any new arguments ?

hyc: it isn't about good/bad practice, it's about understanding what is going on; in the worst case bad/good practice may be not appropriate

<sech1> "so it's correct that get_block_t..." <- so correct that it was required to move "lock" from rpc into blockchain class

regarding 7937, it needs to be checked for other places where lock duration can be reduced

monero-project/monero #7936, it should be replaced with more consistent methods for core/blockchain classes since it deeply depends on their internals

and not with "locks" in rpc

sech1: no, reverted and then added separate methods for core/blockchain which will hold appropriate locks where it's needed without inconsistency

sech1: it isn't about performance optimization to reduce time for lock being held, but about consistency in api (it's much bigger problem in scope)

> <@sech1:libera.chat> regarding 7937, it needs to be checked for other places where lock duration can be reduced

* it isn't about performance optimization to reduce time for lock being held, but about consistency in api (it's problem with much bigger scope)

who said anything about performance optimizations? I meant lock scope, not duration. This issue was particularly because lock order was wrong. Every place where DB read lock is taken needs to be examined to check where the data is being updated and what locks are held in those places.

<sech1> "regarding 7937, it needs to be..." <- just think: core is an abstract lib with all hard calls that could be used by different apis (rpc / zmq / ...), these calls should be made consistent within core, not within rpc

* / ... / p2p), these

selsta moneromooo ^

sech1: 7937 added "locks" blindly, now you're placing one of them into right place and asking for review and ignoring bigger issue which is clear from problem investigation; facepalm

funny that 7937 started from monero-project/monero #7936, which started from few irc comments; just facepalm

There's no bigger issue. Added you on ignore again, you're unbearable. Look, we all know that you like facepalming and solving everything by rewriting from top to bottom. Just stop. While you're facepalming, I solved the issue at hand.

sech1: zmq is an api but called within core, rpc is an api but called outside of core; at the same time someone is adding db "lock" into an api (rpc) in order to have consistent view on core calls, but different api returns different view; it is inconsistent behaviour

<sech1> "There's no bigger issue. Added..." <- solved one small scope issue, but advocated for preserving of incorrect patch + advocated for not wise fixes in long term for similar issue + not admitted that 7937 is a wrong way; it's more harm than good

the problem is that it isn't just you, but many things at once, and when I'm talking about one of them it leads to pointless ping-pong where no one is responsible for anything

sech1, even if you're acting in good faith it allows to hide some problems, it's bad in long term

<sech1> "There's no bigger issue. Added..." <- btw, "it's from bottom to top"; hahahahaha

<sech1> "There's no bigger issue. Added..." <- and it isn't applicable for everything, in that particular it's possible to do small careful changes before full rewrite, but not blind changes in high layer without understand low layer

what is wrong with wallet2???

s/"//, s/"/, nobody is doing rewrites from top to bottom/

Windows Defender saw that as a virus

idk why now

but it was not recognizing wallet2 as a virus

but it did

it looks like reproducible problem, try to bisect

maybe about runtime security but it is interesting because

it was same

<sech1> "There's no bigger issue. Added..." <- btw, +1 for finished bisection, it was useful work anyway

my code was same to yesterday

and this morning Windows Defender decided it is a virus

i rebuild it after that

it was working

but now it is not getting blocks

Monero made me really crazy

MeowingCat: can you try to put more of your thoughts in one comment? Instead of 100 separate sentence fragments. Also, filter out the ones we don't care about

you can try not to be rude instead

MeowingCat: that's just personal spam protection before adding someone into personal ignore list, not global rule

Still. Put your thoughts together before hitting the enter button.

Tip: use periods [ . ] MeowingCat: