#8427 please 🙂

TrasherDK[m]: thought about it but it's still quite new, would prefer in the next release

it would be great if you can run it locally for multiple days and report back if you find any issues

Okay 😢 Can I pull this into 17.3.2 ? I can't build master.

selsta: have you tested 8426? (Jbermans 7760 + pr comments)

ofrnxmr[m]: IMO it's good to merge but we will wait until the second release to include it so that it can get one last review

if no one reviews it until the second release we will just include it

second release will be less than a month away

Nope. Can't pull 8427 into release-v17

jeffro256[m]: do you know what is the minimum openssl version required to work together with the 3.0 api?

^ forget my question, seems we need 1.1.1 or newer

.merge+ 8435

Added

.merges

release day? :)

did a GUI test build and with 8435 everything looks good

jberman is doing some more testing for 8433

but we can merge, branch and tag once luigi becomes available

any chance 8426/7760 gets included in first release to make sure that all nodes run it

some people might just skip a point release

if its getting merged in 2nd release regardless or review or not

s/or/of/

since hardware wallet support will be added with the second release a lot of users will upgrade anyway

i mean if it's up to me i'm fine with merging it now, but not sure what others think

Hello, I use the library `crypto.js` on `xmr.llcoins.net/addresstests.html` to generate the address.

But when I run this function 70,000 times it throws this error: `Cannot enlarge memory arrays. Either (1) compile with -s TOTAL_MEMORY=X with X higher than the current value 16777216, (2) compile with ALLOW_MEMORY_GROWTH which adjusts the size at runtime but prevents some optimizations, or (3) set Module.TOTAL_MEMORY before the program runs.`

which I found in the `crypto.js`. I am not an expert in C++ but it seems the `Module._free` step failed and there is a memory leak.

Anybody knows a memory safe way to do the `sc_reduce32` step? Another js library?

<willshu[m]> "which I found in the `crypto.js`..." <- can you post a link to the repo?

I'm curious. How does this fit with lifecycle/garbage collection of the environment. 70K malloc/free in a tight loop, fells unhealthy.

<selsta> "i mean if it's up to me i'm fine..." <- Im ok with it now.... (full message at libera.ems.host/_matrix/media/r0/do…4d31b9fe6f17d1b45e614a567c787f9c481)

> <@ofrnxmr:monero.social> Im ok with it now.... (full message at libera.ems.host/_matrix/media/r0/do…8b31fbd58dc1c4652656d1d2d026a15f47b)

ping for irc vtnerd__ ^^ see ofrnxmr[m]

is anyone else having github loading issues? some pages are all jacked up on this laptop

the trouble I had with that connection fix issue was that there were too many "flags" and unnecessary code it seemed like

but that was at a quick glance. I still cant get the page to load

the ssl portion could also be done separately iirc

but wasn't completely fixed either as the loop was still racy if a throw occurs

yeah I thought it should be separated from day 1 ... and seriously the page loads fine for you? wth is with firefox on this machine :/

Regarding:... (full message at libera.ems.host/_matrix/media/r0/do…7ff734312485fbaab1ced8df5aefa7b8741)

vtnerd__: the page is loading on my end, but i sent the relevant comments above

yeah I fixed it, an incorrect date on thix box was causing partial ssl issues and so I was getting partial loads on github

yeah I guess it was a style thing; i just didn't like it as it seemed too cluttered

I'll look again after lunch, but there this should be a _potential_ dangling reference issue on throw; but it depends on the caller code so its extra annoying

fixing that required a much bigger chance, and I didn't even attempt that

*change

Jberman can comment further, but I believe perfect daemon plans to continue improvements.

But as it stands, without 7760 and 7999 my android nodes get killed in under 5 days, regularly. Or completely stop accepting connections with rpc-SSL=enabled

So, im not sure there are any drawbacks to merging.

vtnerd__:

This is the updated version that addresses your original review

vtnerd__: where is the potentially dangling reference defined?

ssl_options_t::handshake is given a reference to an asio socket

several of the callbacks will use this reference, but there's no "move" into a shared_ptr to guarantee that it remains live

so `poll_one` in that loop could throw while a callback is still active

whether it gets called on another thread depends on the caller setup, and multi-threaded, etc

iirc its plausible (but unlikely since there aren't _many_ exceptions) on server side only

client side should luck out

Im not 100% sure whether its possible due to way the server-side connections work, but its "dicey" just looking at the single code snippet

its not worse than the existing code though (and improvement, just not 100%)

Interesting, so if a reference is exposed to multiple threads and one thread throws, the throw will unwind the callstack and kill the reference but the other thread isn't aware and keeps using the resource?

the throw wouldn't kill the other threads?

Although I guess the stack unwinding itself could be a race between resource access and thread lifetime

or the threads could be defined above a try/catch

instantiated*

what were the block rewards in monero blocks 0 (genesis) and 1 ?

tromp: try a block explorer.. xmrchain.net/search?value=0

i tried, but the results don't make sense, so I ask here

maybe you need a different question, because the block explorer has the answer

it reports 17.592186 for genesis reward

while i computed it as 17.592186044415

hmm maybe it's rounded?

where can I see unrounded rewards?

tadah click on the miner tx xmrchain.net/tx/c88ce9783b4f11190d7…2200f9faaee8e98dd07e6811175177139/1

explorers should not be rounding amounts

ah, thanks

for block 1 i computed it as 17.592152489984

but that explorer gives 17.592169267200

which appears to be too much

(maxBound::Int64) - 17592186044415 = 9223354444668731392

and 9223354444668731392 >> 19 = 17592152489984

you should use uint64

oops

(MAX_UINT64 - 17592186044415) >> 20 = 17592169267200

IIRC first blocks used >> 20 because it was 1 block per minute

yep, i confused Int64 with UInt64 :(

Reading a little more, threads don’t propagate exceptions to other threads (unless you make extra effort to do so). So would this case be when the main thread throws and kills a resource that other threads hold a reference to?

is it possible that miners claim less than the full reward? and thereby change the following rewards?

it was possible, but not anymore

I don't remember which hardfork disabled it

#define HF_VERSION_EXACT_COINBASE 13

so v13 hardfork, 2020-10-17

block 1964 claimed a reward of 17.525041498439

while 17.55926641562 was allowed

so the max reward at given height can only be computed while replaying the whole history of coinbases?

yes

also because of block weight penalties

when is supply expected to overflow 64 bits?

may 2024 or so

the reason for my reward calculations is, i'm trying to compute Monero's Average Soft Emission Time

how long after launch the average soft monero is emitted

Tromp #monero-research-lounge:monero.social

.merges

rest are sub 24 hours or are semi-final housekeeping

UkoeHB: , jberman: and vtnerd__:

What do you guys think about 8426 (7760), good to go?

vtnerd__: the dicey-ness of `poll_one` throwing and the socket destructing seems present in the code before this PR too

Considering the PR does solve a dicey issue along similar lines (I'd argue the seg fault it solves seems dicier since it doesn't rely on a boost internal exception to trigger), I think it's worth shipping, even if it doesn't fully solve all dicey-ness

I understand why it would be preferred for a larger rewrite to tackle all issues like this, but zooming into just the handshake, it's not that much code in isolation (the handshake change is ~130 lines)

Even if the PR for just the handshake was separate, I would argue it's worth shipping today

selsta et al, when do you want to branch?

ofrnxmr[m]: idk anything about 8426, just interested in the dangling reference from a c++ learning point of view

only question is 8426, it has been tested a lot now by multiple users and it seems to work perfectly

even with this worst case dangling reference remaining

luigi1112: do you think you can branch and tag before you go to sleep?

until then we will decide what to do with 8426

today? sure

and then gui tomorrow

as soon as it's tagged, we can start reproducible builds, right?

yep

since we have new macos arm binaries we will also have to do some changes to the website

nice, I've always enjoyed the privilege of running the release binaries days before everyone else :D

Regarding 8246, my humble take: Reviews in honor, but it also must mean something if code runs flawlessly for many people for months

i think i'm running it for close to year now

And you would think with a coin stealer in there jberman would have found it when reviewing :)

gingeropolous @gingeropolous:monero.social: runs as well (on a high bandwidth node) I believe?

yeah, i've run that for a while now. wish i had an uptime log

thats the same as master beta right selsta ?

Yeah

The worst case with this code is that it introduces a way to much more easily DoS nodes than it already is. It patches some DoS vectors and clear daemon reliability issues as it stands, so arguably, not including the code is close to the same as the worst case

Obviously it's also a huge section of complex code and if there was a clean way to get ALL the benefits of this PR in a clean way, I would be all for it. I sat with it. I tried. I discussed with perfect-daemon. And after doing so, I arrived at the conclusion that the way it's structured has underlying reasoning that is justifiable

I'm for merging for the above reasons

as a random idea, it could go in the immediate point release. this way, it doesn't get 100% on the network.

i mean because the main job of the next release is to fork the network

well, this release. at least thats how i see it.

but its on xmrchain now

I think we should get 7760 out of the way sooner rather than later (could have been merged months ago but was just lacking a completed review then eventual merge conflicts etc forced a resubmit)... (full message at libera.ems.host/_matrix/media/r0/do…d879bef2c4c303beb72fac0504d57182e95)

I was planning on going through 7999 next

Has anyone actually pledged to review 7760/8426? sech1 vtnerd__ ? If no one plans to review it and more reviews are expected, then it will just go back into indefinite limbo. So either decide 'wait for pledged reviews' or decide between 'no more reviews just merge it' and 'limbo until more reviews are possible'.

UkoeHB:

7760 was reviewed and approved by jberman

8426 is cosmetic changes to address vtnerd style choice

I vote option 2. No more reviews, just merge it.

Rbrunner7 did a fast review of 8246 to ensure no logic changes were made during the cosmetic changes

jberman[m]: correct, as-stated it is no worse, it just doesn't fix all cases unfortunately. I had a separate patch that was simpler than this but stopped when I identified the second issue

or I guess simpler was subjective since it has its own deal of boilerplate

Vtnerd, how to translate that into "are we good to go?"

This patch has been run and tested for a year.

I think the handshake solution is pretty clean/simple as is. It’s just this: monero-project/monero df0c7e6

The read/write handling is a lot more involved. If you want to deeply review the read/write handling further for correction issues, then I think that would justify keeping the PR on hold

If you only see further subjective changes that don’t necessarily affect code correctness, then I think we merge and get the correctness fixes out for the hard fork, you implement the changes you want to see when you’re able, and I’ll review

Will review happily I will add