< achow101>
istm there shouldn't be a lock order issue detected because the function should have exited and the locks released by the time that the different lock order is being used
< sipa>
that's not what lock order detection does :)
< sipa>
it verifies that the same locks are always taken in the same order
< sipa>
if they are _ever_ held simultaneously
< achow101>
ah
< sipa>
it's perfectly possible that it detects inconsistencies that cannot ever result in actual deadlocks
< sipa>
(but that generally does mean the code is fragile, as calling things a bit differently can cause a deadlock)
< achow101>
indeed
< achow101>
does it only detect locks held simultaneously or does it also detect the order of locks taken after one is released?
< sipa>
if locks A and B are ever held simultaneously, it will remember their relative order
< sipa>
if two conflicting relative orders are observed, it will abort
< sipa>
"order" here means "A is locked while B was held" -> "A > B"
< achow101>
ok
< sipa>
so you can't have code that in some place locks A while B is held
< sipa>
and elsewhere locks B whila A is held
< achow101>
and yet we do
< achow101>
but only if you try to do dumpwallet and have loaded the wallet when there is an unconfirmed transaction belonging to it in the mempool
< bitcoin-git>
[bitcoin] achow101 opened pull request #22492: wallet: Reorder locks in dumpwallet to avoid lock order assertion (master...dumpwallet-lock-order) https://github.com/bitcoin/bitcoin/pull/22492
< robertspigler>
luke-jr: makes sense, thanks!
< vasild>
achow101: sipa: but does our lock order detect potential issues like: lock A, lock B, unlock B, unlock A; at this time the lock stack for the thread would be empty, then at a later time: lock B, lock A.
< vasild>
looking at src/sync.cpp I think it will not detect ^
< laanwj>
no, it doesn't detect that, i guess it would require quite a lot of extra record-keeping
< laanwj>
keeping track of current locking state versus historical locking state
< vasild>
right, that would be significant runtime performance overhead
< S3RK>
now I'm confused. What's the difference between "conflicting relative orders are observed" and "locking state versus hisotrical locking state"?
< vasild>
hmm, actually not, the "lock orders" database can be global, need not be per-thread, so at least the size/memory overhead would not be too much
< vasild>
S3RK: "conflicting relative orders are observed" probably means that some thread locked A, B (in that order) and another thread locked B and tries to lock A, _while_ the first thread still holds both mutexes
< vasild>
while "locking state versus hisotrical locking state" imples that we remember that the first thread has locked A,B even after it releases those mutexes, so if we do that it is possible to detect if another thread locks B,A even after the first thread has released all mutexes
< S3RK>
how is it possible that the second thred locked B while the first thread still holds both?
< vasild>
it is not possible, of course, I am talking nonsense. The scenario is: thread1: lock A, thread2: lock B, thread1: try to lock B, thread2: try to lock A
< vasild>
The thing is that after a thread releases a lock we delete it from its lock-stack == we forget that it ever locked it.
< S3RK>
hm... this sounds like deadlock detection. I thought debug_lockorder is something more
< vasild>
see push_lock() and pop_lock() in src/sync.cpp
< S3RK>
will do, thanks
< S3RK>
lockorder is actually removed in DeleteLock() but I'm not familiar with the code at all to understnd fully how it works
< vasild>
but wait, we do erase() from DeleteLock()
< laanwj>
huh, I thought the idea behind using thread-local storage in DEBUG_LOCKORDER was to avoid the overhead of having any global synchronized objects in the lock tracking
< laanwj>
but yeah that can't be the case, it has to cross-check
< laanwj>
it was never intended to be really efficient or low-overhead, that's why it's only enabled with debug builds
< jnewbery>
I think DeleteLock() is called when the mutex is destroyed, not when the lock is released
< jnewbery>
LeaveCritical() is the function called when the lock is released
< laanwj>
that seems to be the case, it is called from the destructor of AnnotatedMixin, which is used to define RecursiveMutex etc
< _aj_>
laanwj: it's not really thread local storage, it's a global map (GetLockData()) indexed by thread id? it used to be thread local prior to #18881 by the looks
< vasild>
This is the list of i2p nodes I am aware of: https://bpa.st/UOXQ, I (tried to) connect to each one and observed whether they advertise themselves with port 0 or 8333.
< vasild>
jonatack: do you have more i2p addresses than listed in https://bpa.st/UOXQ ?
< jonatack>
yes, 3 other i2p ones, otherwise i have the same list
< bitcoin-git>
[bitcoin] laanwj merged pull request #22387: Rate limit the processing of rumoured addresses (master...202106_rate_limit_addr) https://github.com/bitcoin/bitcoin/pull/22387
< emzy>
vasild: I will update my i2p node to master later today.
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #22493: fuzz: Extend addrman fuzz test with deserialize (master...2107-fuzzAddrDeser) https://github.com/bitcoin/bitcoin/pull/22493
< jnewbery>
vasild: in 22112, is there anything stopping peers from gossipping non-port 0 I2P addresses to us? Will those addresses be entered into our addrman?
< laanwj>
IIRC nothing is stopping peers from gossiping non-port 0 I2P addresses, they will get stored (but not connected to, until our I2P SAM support supports non-zero ports, or other software that does). After next start (when CAddrMan::ResetI2PPorts is invoked) the ports will be changed to 0. That's meant to be a temporary measure.
< laanwj>
I sometimes wonder if ResetI2PPorts is needed at all--I mean, the amount of I2P peers that has already been gossipped is bound to be very low, peers will broadcast the new port after upgrading, the old ones will drop from the database at some point
< laanwj>
the old behavior has never been in a release
< jonatack>
It was needed to continue making outbound connections to existing I2P peers with port 8333 in the addrman, which otherwise can no longer be connected to nor (afaik) manually removed from the addrman. The alternative would be for the existing 20-30 peers that have run an I2P service to delete their peers.dat
< jonatack>
or to have all upgraded to current master and restarted, so that ResetI2PPorts() can then be removed before 22.0 instead of at some point afterward
< jonatack>
as it was planned to be a transitory measure
< laanwj>
right
< jonatack>
(I considered adding a hidden RPC to manually remove addresses from addrman, but that seems like an unneeded attack vector)
< vasild>
yeah, I don't have a strong opinion but removing ResetI2PPorts() from final 22.0 looks plausible
< laanwj>
if it's a one-time upgrade, some versioning mechanism that runs it only once (instead of every node start) would make sense, that said, not having it end up in a release at all would maybe be best
< jonatack>
seems most prudent
< fanquake>
I'm not yet caught up on the all new addrman brokenness, but surely we shouldn't be writing code to compensate for code that was already broken when added to master.
< fanquake>
All the bugs should be fixed, and anyone running master just needs to update, delete their peers.dat / whatever. It's not like there is any significant amount of people actually using i2p yet.
< laanwj>
fanquake: right
< fanquake>
Working around broken code in master so as not to inconvinience a minority that are running pre-production software seems backwards .
< jonatack>
istm they need to all delete peers.dat (or upgrade to current master first) though
< laanwj>
could even add a release note, though there may be better ways of reaching the few people who test I2P with bitcoin core master
< jonatack>
yes, i know who all of them are except the bitcorn one
< laanwj>
in any case it's complciated a bit by having ResetI2PPorts in master as well... like, eveyrone who ran master *in between* that function being added and re moved will not need to remove peers.dat :)
< jonatack>
right :)
< vasild>
I like solving problems by deleting code :) especially if the code looks risky/hackish and especially if I wrote it ;)
< laanwj>
vasild: agree, glad you're ok with it
< vasild>
ok, should we leave master for some more time with ResetI2PPorts() or remove it immediately?
< laanwj>
we could have a PR and merge it as last thing before the branch-off
< laanwj>
but I imagine that's going to be soon
< laanwj>
with #22387 merged and the I2P addrman issues solved I don't think there's any blockers left?
< laanwj>
though I am honestly not sure how much of a difference it makes in practice; kernel version dependency is generally an issue for static linking of libc
< laanwj>
libc is supposed to abstract the kernel details from the application; but this might be leaky in some cases
< laanwj>
(for example when the application, or one of the statically linked libraries, manually invokes syscalls without handling the non-present case)
< jonatack>
can open the pull
< laanwj>
i don't think this is the case for us but...
< jonatack>
(vasild: can open the pull)
< vasild>
yes, I will shortly
< bitcoin-git>
[bitcoin] theStack opened pull request #22495: p2p: refactor: tidy up `PeerManagerImpl::Misbehaving(...)` (master...202107-net-tidy_up_misbehaving) https://github.com/bitcoin/bitcoin/pull/22495
< bitcoin-git>
[bitcoin] jnewbery opened pull request #22496: [addrman] Remove RemoveInvalid() and ResetI2PPorts() (master...2021-07-remove-addrman-hotfix) https://github.com/bitcoin/bitcoin/pull/22496
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #22455: addrman: detect on-disk corrupted nNew and nTried during unserialization (master...addrman_detect_negative) https://github.com/bitcoin/bitcoin/pull/22455
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #22498: Check that CAddrMan::m_key is not null after deserialize (master...2107-addrmanKeyZero) https://github.com/bitcoin/bitcoin/pull/22498
< MarcoFalke>
Should addrman related pull requests be tagged with "p2p"?
< jarolrod>
MarcoFalke: circling back on drahtbot and updating it to label addrman changed `P2P`. Maybe it also should remove the `Needs Guix Build` label when it posts its guix build. We still want people to run a guix build and compare hashes right?
< jarolrod>
^ should not remove
< MarcoFalke>
Anyone is free to run as many guix builds as they wish
< MarcoFalke>
Not sure if we should use the label to indicate *someone* (other than DrahtBot) should run the build
< hebasto>
^ yeah :)
< jarolrod>
oh ok i see, it's for drahtbot
< MarcoFalke>
I guess DrahtBot could use "Build" as label to create a guix build
< MarcoFalke>
Not sure if it will be fast enough to keep up, though
< MarcoFalke>
It is running on half a core (one thread)
< jarolrod>
that's ok, no need for change then
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #22502: scripted-diff: Revert "fuzz: Add Temporary debug assert for oss-fuzz issue" (master...2107-fuzzTempRevert) https://github.com/bitcoin/bitcoin/pull/22502
< Guest85>
Hey guys! I'm new here, just a quick question?
< laanwj>
i guess we could rename the label to something like Drahtbot: GUIX build, if that's more clear
< laanwj>
i understand the confusion because the other 'needs...' labels imply a contributor has to do something
< sipa>
drahtbuild-requested
< jarolrod>
^^ I had been using that label to find pr's that need a guix build done :)
< MarcoFalke>
Sure, will rename
< jonatack>
fwiw seeing 4 new I2P addresses since a few hours ago and 5 inbound I2Ps advertising port 8333 (running with marco's ADDRMAN_DEBUG assert branch)
< jonatack>
s/running/i am running/
< jonatack>
never mind about the port 8333 part; i had not noticed that the ADDRMAN_DEBUG assert branch was before the I2P Port 0 merge...rebased, rebuilt, restarted, no inbound I2Ps with port 8333 -> to bed