< achow101>
it seems that I am unable to ban. For some reason the string for the node's address is empty
< achow101>
so how's the prefinal alert doing?
< achow101>
gmaxwell: ^
< * luke-jr>
wonders why testnet rewinds blocks every time he runs it
< gmaxwell>
luke-jr: thats the chainstate check.
< luke-jr>
oh, somehow I was thinking it was for segwit
< GitHub87>
[bitcoin] luke-jr opened pull request #8877: Qt RPC console: history sensitive-data filter, and saving input line when browsing history (master...qt_console_history_filter) https://github.com/bitcoin/bitcoin/pull/8877
< wumpus>
aureianimus: if you don't fix your IRC client I'll have to ban you to reduce join/part noise in this channel
< GitHub70>
bitcoin/master 905bc68 Cory Fields: net: fix a few cases where messages were sent rather than dropped upon disconnection...
< GitHub70>
bitcoin/master d93f0c6 Wladimir J. van der Laan: Merge #8862: Fix a few cases where messages were sent after requested disconnect...
< GitHub157>
[bitcoin] laanwj closed pull request #8862: Fix a few cases where messages were sent after requested disconnect (master...fix-disconnect-send) https://github.com/bitcoin/bitcoin/pull/8862
< BlueMatt>
i mean if a failure to teardown in the right order does anything more than cause memory to keep growing as you push events that arent handled, then you have other issues
< cfields_>
wumpus: thoughts on the above? IIRC we've discussed briefly before. The question is: what model do we use to glue networking and message handling together?
< cfields_>
BlueMatt: i don't see the problem with the approach before the shared_ptr? As long as a signal is broadcast to the processors that the connman is no longer valid, that seems fine to me?
< BlueMatt>
before the shared ptr? you could delete and dereference null, no?
< BlueMatt>
i mean if you failed to teardown, that is
< wumpus>
well if it is CConnMan that manages the lifecycle of connections there's not need for shared_ptr
< cfields_>
BlueMatt: right, with the caveat that the handler would have to appropriately act on the signal in time
< wumpus>
you may need 'weak-pointer' like callbacks in that case, to prevent anyone still holding keeping handle when a connection goes away
< BlueMatt>
wumpus: not about connections, about the CConnman itself
< wumpus>
Init() and Shutdown() 'own' CConnman
< cfields_>
BlueMatt: then CConnman will have advertised that all connections have been torn down before it itself is deleted
< wumpus>
we have a well defined lifecycle there
< wumpus>
after Shutdown there should be no networking anymore, and no connman
< wumpus>
and everything that uses connman should be deinitialized before ConnMan gets deinitialized
< wumpus>
better to have deterministic lifecycles for objects then rely on shared pointers to happen to get things right
< wumpus>
or even worse, relying on global constructors/destructors order
< cfields_>
+1. Though I think also it's important to design such that net users essentially have no chance of trying to send messages by the time CConnman has shut down, so that the fact that it's "owned" by Init really doesn't matter.
< cfields_>
eg. queued messages should be deleted when the nodes are advertised as disconnected, and processors should be shutdown when their connman advertises that it's stopping
< wumpus>
that depends on what preconditions you try to enforce, if net users remain by the time CConMan shuts down you'd say that's a bug
< wumpus>
oh right, it may be a multi-phase process
< luke-jr>
wumpus: I wasn't objecting to -getinfo <.<
< cfields_>
right, the processor will get an interrupt which should break its loops, then a blocking stop, after which it should assume that its connman is null'd
< wumpus>
luke-jr: I know, but I just don't think it's worth pushing too much for, it's something I'd use myself but I don't think anyone else cares and maybe I should just get used to using the respective commands, or simply make a python script
< wumpus>
heck or just a bash alias. bitcoin-cli getwalletinfo && bitcoin-cli getnetworkinfo | head -20 or such. I'd been overthinking it a bit because I thought it was neat to do RPC batch from bitcoin-cli
< cfields_>
wumpus: you mentioned in #8856 that you'd like to see an api for options eventually. I have a few generic base classes that I've been working on in order to break some global dependencies out of utils. for threading, logging, options, etc. Wasn't sure if you'd want those or not. I could PR an RFC if you think it's worth discussing
< cfields_>
(it was done as an exercise for CConnman, seeing what it would take to drop all global state deps by passing in a base logger, base options parser, etc)
< wumpus>
cfields_: yes I think that would be a good thing, options handling has always been somewhat of a mess. And when boost has to be dropped as a dependency we need a replacement for boost::options.
< cfields_>
wumpus: right
< cfields_>
wumpus: seems the only thing we actually use it for is parsing the config file?
< wumpus>
cfields_: there's a lot unclear though, e.g. do we want to be able to change (some) options on the fly (which would require callbacks), how to handle types, how to handle defaults, etc
< wumpus>
yes, just replacing it would be trivial, doesn't need a whole options overhaul :)
< cfields_>
wumpus: as a stopgap, I just kept it all pretty much as-is
< wumpus>
e.g. how we handle defaults is kind of ugly. In stead of declaring the option and its type and default e.g. at the top of the module where it gets used, we have to declare a constant DEFAULT_BLABLA then pass it in in every place where the option gets used
< cfields_>
wumpus: stores a const map of strings, a const multimap, then a non-const map for the soft-sets. The getters treat that as an overlay, and fallback to const
< wumpus>
also there's a lot of overhead as arguments get parsed to their type time after time, in some places
< wumpus>
and it's possible to call GetBOol on an argument in one place and GetString on it in another place
< cfields_>
wumpus: heh, yes. There's certainly the argument that the options shouldn't even be visibile outside of init
< wumpus>
cfields_: that would be one way, though it means all the argument propagation logic is centralized. Right now we are moving towards having the wallet handle its own arguments, for example, which makes sense to break the circular dependency and get rid of ENABLE_WALLET defines.
< wumpus>
it's in no way a trivial thing to get right
< wumpus>
many constraints, and low priority, and the current state well... kind of works
< wumpus>
cfields_: yes, validation would be good :)
< cfields_>
wumpus: low priority, hint taken :)
< wumpus>
well the net refactor is much more important
< wumpus>
as well as build system/gitian security
< wumpus>
speaking of which how's your mac code-signer project doing?
< sipa>
wumpus: i once started on an options system (inspired a bit by google's, where you just declare a global IntOption nMaxConnections("-maxconnections", minval, maxval, description); or so, and then can query nMaxConnections as if it were an integer
< cfields_>
wumpus: got distracted when some net stuff got merged. It's fully working and stable though, the only work left is to figure out the detach/attach scheme
< sipa>
i also think luke started on an options thing where they could be changed over time
< wumpus>
sipa: sounds good to me
< wumpus>
and yes luke-jr has some pulls open that accomplish some of these things
< wumpus>
haven't looked very much yet at those, sorry
< cfields_>
1 pre force-push, 1 post. No surprise, they match :)
< wumpus>
good
< BlueMatt>
cfields_: sorry, had to step out...do you prefer, then, that we stick with the map<pointer, thing>?
< BlueMatt>
I mean I'm open to that, you just objected that it was fragile, and shared_ptr is an explicit fix for fragility of using a ptr there
< cfields_>
BlueMatt: for the sake of not holding it up, sure.
< BlueMatt>
I mean, ok, I'll revert the shared_ptr change, then, but what would be the long-term target there? a logic class which owns the CConnman?
< GitHub196>
[bitcoin] laanwj closed pull request #8746: [Qt][RPC] Hide passphrases in debug console history (master...hide-walletpassphrase) https://github.com/bitcoin/bitcoin/pull/8746
< cfields_>
BlueMatt: it doesn't own it, it just holds a reference to one. It should receive signals that the connman is being torn down, and assume that it's invalid from that point forward
< BlueMatt>
cfields_: hmm, ok...I find that harder to reason about (and it generates more code), but, ehh, whatever
< BlueMatt>
cfields_: in any case, thats not something that is less than a lot of work to make happen
< cfields_>
BlueMatt: well, lots of things are going to want to own the CConnman. They can't all have it :)
< sipa>
well either it's owned by one thing and one thing only, or it's refcounted
< BlueMatt>
cfields_: oh? what else wants to own CConnman? I think not much else cares about anything at a lower level than what peers are doing
< BlueMatt>
cfields_: aside from the need to provide the setup argument sin init
< cfields_>
BlueMatt: rpc/wallet/gui could all make the argument
< sipa>
i'd say init creates&owns CConnman, and passes it as argument to rpc/wallet/gui
< sipa>
who hold a reference to CConnman, but don't own it
< sipa>
and it's init's responsibility to not shut down CConnman before all its users are destroyed
< BlueMatt>
cfields_: why does rpc/wallet/or gui care about anything more than the list of nodes connected to, outside of the context of configuration options
< sipa>
(please tell me to shut up if there are large issues i'm not aware of, i haven't actively followed the code)
< cfields_>
sipa: agree, that's what I'm suggesting as well
< BlueMatt>
sipa: ok, well thats what it is now
< BlueMatt>
cfields_: was taking objection to that
< sipa>
BlueMatt: how so?
< cfields_>
BlueMatt: eh? I'm aligned with that. I'm taking objection to a processor taking ownership
< BlueMatt>
"a processor"?
< BlueMatt>
you mean the PeerValidationLogic
< BlueMatt>
it doesnt take ownership, it keeps a pointer to the object
< cfields_>
message handler, validator, etc
< BlueMatt>
and " it's init's responsibility to not shut down CConnman before all its users are destroyed" :p
< cfields_>
BlueMatt: yes, and I'm fine with that :)
< BlueMatt>
wait, now I'm confuseg
< BlueMatt>
ugh, ok, I'll leave it how it is
< cfields_>
just not a shared_ptr
< sipa>
well ideally, the PeerValidationLogic is a class, and there is a seaprate instance of it for each CConnman
< sipa>
so it can hold a pointer, and does not need a map
< BlueMatt>
cfields_: yea, I'll remove that
< BlueMatt>
sipa: it does, the map is only as a place to put the storage, mostly
< sipa>
BlueMatt: i'll shut up before looking at the
< sipa>
code
< BlueMatt>
its just there so you can do Register(CConnman*) and Deregister(CConnman*) and it knows which PeerValidationLogic object is associated with that connman so it can destroy the right one
< sipa>
hmm, shouldn't init create the PeerValidationLogic object, and just pass it the CConnman pointer?
< sipa>
there should never be a need to look something up
< BlueMatt>
How else do you know which PeerValidationLogic to destroy when someone hands you a CConnman* to Deregister()?
< BlueMatt>
i mean aside from iterating
< BlueMatt>
anyway, whatever, could also iterate, doesnt matter much
< sipa>
init would create the PeerValidationLogic
< sipa>
and destroy it
< BlueMatt>
ahh, yea, i mean thats maybe a more forward-looking thing...once the code is well-separated and designed, maybe, but for now it lets main store it
< sipa>
ok
< cfields_>
BlueMatt: ^^ seems simpler than stashing it in main?
< sipa>
all good; "it requires too much refactoring" is a good reason
< BlueMatt>
cfields_: I didnt really want to expose the PeerValidationLogic in main.h yet
< sipa>
gah stupid github and its author date based ordering of commits
< BlueMatt>
i had originally started doing what sipa said, and then decided i wanted less exposing of shit
< sipa>
it took me 10 minutes to figure out why #8871's commits didn't show up in #8393
< sipa>
turns out they are there... at the end
< cfields_>
BlueMatt: mm, ok. Giving you a pass since the intention is to break stuff out anyway
< cfields_>
:)
< BlueMatt>
cfields_: heh, ok, will need like 2 more prs for that :p
< cfields_>
BlueMatt: wait, isn't it a child of CValidationInterface ?
< BlueMatt>
is what?
< cfields_>
yea, PeerLogicValidation
< cfields_>
why not just keep a base pointer in init?
< BlueMatt>
oh, you mean ptr to CValidationInterface
< BlueMatt>
hmm, dont we need like a virtual destructor or something crazy like that?
< cfields_>
er, setup a base ptr to a dummy interface in init, then actually create it later in the process
< BlueMatt>
wait, what?
< cfields_>
oh, it's already non-virtual anyway. no need for the dummy
< cfields_>
BlueMatt: nm, it can wait 'til later
< * BlueMatt>
is so confused
< cfields_>
BlueMatt: i can write it up real quick
< BlueMatt>
ugh, y'all and your C++ magic
< sipa>
BlueMatt: for my grammar-based password generator, i wrote an stl-like list where the iterators are refcounted, and elements automatically get destructed when no iterators remain... epic fun with rvalue references everywhere :)
< BlueMatt>
fuck you people
< sipa>
though i don't think cfields_ is talking about anything as extreme as that :)
< cfields_>
sipa: haha
< cfields_>
no, I just mean using a base class as a base class :)
< cfields_>
BlueMatt: the unique_ptr is awkward because it has to be passed to main for creation. Once the header can be exposed, nearly all of that ugliness goes away
< BlueMatt>
the second I see std::move my eyes glaze over
< cfields_>
eh?
< BlueMatt>
you're like passing around something without knowledge of the type and using std::move and things
< BlueMatt>
i really dont like that
< cfields_>
huh? Yes, I'm using virtuals and rvalues...
< cfields_>
Everything there is type-safe and defined
< cfields_>
but as mentioned above, that's only there to work around the fact that the header isn't exposed. The moves go away once you can create the inteface directly in init
< BlueMatt>
i mean you had to add a virtual destructor......
< BlueMatt>
was mostly what i was referring to
< BlueMatt>
can we just push this off until its in net_processing.h and we can expose the class there?
< BlueMatt>
like, ehh, you're bending over backwards to make this work without exposing the PeerLogicValidation, whereas I'd kinda prefer to expose it in main.h
< BlueMatt>
though, mostly, Id rather put it off into peer_validator.h
< BlueMatt>
ehh, net_processing.h is what i was calling it
< cfields_>
BlueMatt: I'm just coding it up with the constraints I've been given. Obviously it's much nicer to just split out the header and use it directly
< luke-jr>
BlueMatt virtually self-destructs.
< * luke-jr>
hides
< cfields_>
so if that's the plan, sure, no need to do it now
< BlueMatt>
yea, sorry, i mean my "dont want to expose this" is a weak preference just because main.h is Too Damn Big (tm)
< BlueMatt>
after the split i see no reason why it shouldnt be exposed (net_processing.h right now is like 4 functions and some forward-declarations)
< cfields_>
BlueMatt: ok, how about this then...
< cfields_>
stuff it in main.h for now, hold it cleanly in init, then split it out of main.h as the next step?
< cfields_>
that way the plan is clear all along. Or is there a reason that doesn't work?
< BlueMatt>
sure, can do that, too, then I'll just move all the to-be-moved stuff together - RegisterNodeSignals, UnregisterNodeSignals, ProcessMessages, SendMessages, GetNodeStateStats, and Misbehaving
< cfields_>
BlueMatt: that sounds much better :)
< BlueMatt>
since main.h moves are easy
< BlueMatt>
ok, I'ma overwrite the shit out of history on that pr, then
< luke-jr>
lol
< * cfields_>
salivates at the thought of PeerLogicValidation::mapNodeState
< BlueMatt>
heh, yup
< sipa>
at some point i thought about a generic NetmessageProcessor<state> class you could inherit from (with your own state type filled in), which you could register to net
< cfields_>
maybe this can be the final home for CChainParams as well.
< sipa>
and you'd have separate instances for dealing with tx relay, block relay, ping, addr management, ...
< cfields_>
sipa: state types being?
< cfields_>
ah
< sipa>
cfields_: well for the ping handler, the state would be a struct that remembers the last sent ping
< sipa>
for addr management it would be the addrKnown and tosend vectors
< sipa>
etc
< sipa>
and you'd have the ability to do parallel processing across different nodes as long as it was about different handlers
< cfields_>
hmm, interesting
< cfields_>
how to order dependent messages?
< sipa>
you don't
< cfields_>
(for ex the ping ordering that ruins everything)
< sipa>
you only ever process the oldest unprocessed message for each peer
< BlueMatt>
we need to discuss ping ordering at some point
< BlueMatt>
sdaftuar: pointed out that its pretty much violated everywhere wrt block processing
< cfields_>
oh, gotcha. I missed the "different nodes" part. My brain turned that into "across different messages"
< BlueMatt>
essentially, i think we need to grep bitcoinj and other spv clients, find all the places where they actually use ping sync, define those, than then say that that is the protocol
< sipa>
BlueMatt: heh, i thought we'd just stick to synchronous processing all messages
< sipa>
that's such a huge change to deviate from
< sipa>
i'd rather redo the p2p protocol from scratch if you're going to change that
< BlueMatt>
sipa: we already violate it a) with block processing sometimes (sdaftuar thinks this is the cause for some of the compact block test failures, last I heard) and definitely on reject messages
< BlueMatt>
sipa: so we kinda already deviate from it :/
< sipa>
we don't
< sipa>
when receiving a block, we *process* the block fully
< sipa>
and send all messages in response to that
< BlueMatt>
sipa: there are several things we send in SendMessages instead of ProcessMessages
< BlueMatt>
which will violate it
< sipa>
that doesn't mean we fully validate
< sdaftuar>
sipa: block announcements are delivered asynchronously (as are block reject messages)
< sipa>
sdaftuar: sure
< BlueMatt>
cfields_: hope you like your main.h with some #include "validationinterface.h" :p
< cfields_>
oh wow, i didn't realize we'd slimmed the main.h includes down so much
< BlueMatt>
yea, they're pretty minimal now
< BlueMatt>
I dont really mind adding validationinterface, though...we'll kill it soon :)
< cfields_>
BlueMatt: i think that's fine as long as it's temporary
< cfields_>
BlueMatt: any reason not to simply register/unregister in PeerLogicValidation's ctor/dtor?
< BlueMatt>
layer violation?
< BlueMatt>
feel yucky to me
< BlueMatt>
yea, it feels yucky because "hidden magic"
< BlueMatt>
the zmq one doesnt do that
< cfields_>
hmm, as long as the registration is global, it all seems like the same hidden magic to me.
< cfields_>
but np, was just a thought
< BlueMatt>
true, but the registration will eventually not be global :p
< BlueMatt>
i guess I'm kinda thinking about chunks of code as if they were classes even though they are currently not
< BlueMatt>
since they should be eventually, or even if not they'll be well-isolated so even if not a class, a single common interface feels similar
< cfields_>
ok, then future request: when you're registering to a signagls instance, and you pass that instance into PeerLogicValidation, make it RAII please :)
< BlueMatt>
maybe we should just use weak_ptrs for signal instances :p
< cfields_>
and I think i see what you mean, and sounds good
< cfields_>
I really wish there was some unique_ptr with a weak_ptr, it would be really useful sometimes. Though some of the semantics kinda make no sense
< BlueMatt>
yea, would be cool to not have to use shared_ptrs, i guess...would just be somewhat oxymoronic to use it with a unique_ptr
< cfields_>
Well the interesting use-case would be: I'm using a unique_ptr, grab it and hold it if it's non-null.
< cfields_>
so basically a shared_ptr with a max refcount of 2
< BlueMatt>
yea
< GitHub126>
[bitcoin] jnewbery opened pull request #8881: Add some verbose logging to bitcoin-util-test.py (master...verbose-bitcoin-util-output) https://github.com/bitcoin/bitcoin/pull/8881
< ill>
abovetghelaw
< GitHub126>
[bitcoin] sdaftuar opened pull request #8882: [qa] Another attempt to fix race condition in p2p-compactblocks.py (master...fix-race-again) https://github.com/bitcoin/bitcoin/pull/8882
< GitHub177>
[bitcoin] jnewbery opened pull request #8883: Add all standard TXO types to bitcoin-tx (master...add-p2sh-segwit-options-to-bitcoin-tx) https://github.com/bitcoin/bitcoin/pull/8883