< gmaxwell>
interestingly one is on an outbound tor peer, another is on an inbound local peer.
< gmaxwell>
oh.. right this host is running core in valgrind.
< wumpus>
gmaxwell: there used to be a problem where minping was not initialized
< michagogo>
I kicked off my gitian build script last night before going to bed. Linux and Windows completed without any problems, but the OS X build failed
< michagogo>
It went as far as building protobuf, but then failed when extracting boost
< wumpus>
I managed to build all three - what's the issue?
< michagogo>
Tons of error messages from tar, cannot mkdir/open: read-only file system o_O
< wumpus>
gmaxwell: or is that std::numeric_limits<int64_t>::max()?
< wumpus>
yes it is. Okay that means that there hasn't been a ping ever
< wumpus>
unfortunately JSON in their infinite wisdom didn't take into account special floating point values like inf, NaN, -inf, so we cannot use those to indicate 'no ping ever'
< gmaxwell>
should we leave the field out in that case? we do that elsewhere for not-applicable; (I somewhat dislike this as it'll end up with a rare corner case that applications will not handle)
< wumpus>
and using a string or 'null' where people expect a value may be a bit mean (though a good way to check input validation)
< wumpus>
in any case whatever the behavior it needs to be documented in 'help' of the RPC call
< wumpus>
but hey, it's better than uninitialized memory, which it used to be before #6636
< paveljanik>
what about using negative value instead?
< gmaxwell>
then it probably sorts in the wrong position.
< paveljanik>
if it is not excluded from the sort as evidently invalid value, yes.
< wumpus>
it's not a good special marker. I think we return a negative result to mark 'unavailable information' in the fee estimaton calls and this confuses everyone, consistently
< wumpus>
I think sipa's 'null' makes most sense
< wumpus>
deleting the field completely may result in people complaiing 'oh noo why is this field not here it is documented!'
< wumpus>
do we have a way to make a node stop syncing at a certain block?
< wumpus>
oh I guess syncing until tip and then -connect=0.0.0.0 will do the job
< jonasschnelli>
wumpus: I was also looking for this some time ago (for the UTXO set dump). I hardcoded something.
< wumpus>
yes my idea now is to use one 'reference node' which does not receive new blocks, and make the other nodes sync from there
< wumpus>
i'm also comparing utxo set dumps :)
< jonasschnelli>
morcos: ping. Tell me when you have a couple of minutes to discuss the "new wallet" concerns.
< wumpus>
so something like my 'label API' would be new wallet only?
< jonasschnelli>
wumpus: Yes. I think it would be better.
< jonasschnelli>
Because we don't need to take care about the account compatibility.
< wumpus>
on the other hand it may give people a stepping stone to the new wallet
< wumpus>
I agree
< jonasschnelli>
But I'm fine with your PR for the old wallet.
< jonasschnelli>
I just think we should merge the new wallet as soon as it make sense (not now) and go with the tiny steps (PRs).
< jonasschnelli>
Peer reviews is really something that improves the quality.
< jonasschnelli>
And developing it on a different branch will lead to _no_ peer review and a very big PR if we consider merging it back to the master repo.
< jonasschnelli>
Merge ready could be: 1) remove accounts, 2) switch from BDB to LogDB, 3) simplify update logic
< wumpus>
yes we should at least prevent the infinite delay problem that haunts wallet changes, and just merge it without the idea that the API is stable
< jonasschnelli>
Yes. Just a code base where we can work on more aggressively and don't need to take care about every backward comp. API stableness.
< wumpus>
with the understanding that it will likely just be disabled for the 0.13 release
< wumpus>
or super-experimental
< wumpus>
depending on where things are
< jonasschnelli>
wumpus: yes. Certainly disabled for 0.13. My main short term focus is a new wallet codebase in the main repository to allow peer reviews.
< jonasschnelli>
Deploying it to the broad user base is something different.
< jonasschnelli>
Could be in 0.14, 0.15, depending on the progress.
< wumpus>
right
< jonasschnelli>
If it turns out as a bad idea, we can always remove it without loosing anything.
< jonasschnelli>
(before deployed)
< wumpus>
agree
< jonasschnelli>
wumpus: re: --enable-debug-lockorder, hmm... is there no use case to log the maybe-deadlocks but no assertion that kills the app?
< wumpus>
jonasschnelli: hey what a good idea
< wumpus>
yes that may be best, unless it kills the application in a logging flood of deadlock notifications
< michagogo>
10:11:40 <wumpus> do we have a way to make a node stop syncing at a certain block?
< wumpus>
but that's not my experience
< michagogo>
Does blacklisting do that?
< wumpus>
michagogo: but you can only blacklist a block *after* you have it, or not?
< michagogo>
wumpus: that's what I'm not sure about
< michagogo>
If that doesn't work, it should be made the case that it does :P
< wumpus>
it's worth a try, I considered it but then rejected it on that basis, maybe just knowning about the header is enough
< wumpus>
jonasschnelli: I vaguely remember we used to have those deadlock notifications non-fatal, then they would turn up here and there and people would report them but just carry on, it was less obnoxious than crashing, though still if it's false alarms ... :-)
< jonasschnelli>
I think we should --enable-debug-lockorder to enable the whole detections,.. and maybe just remove the fatal assert? But sipa said he know how to solve this. So i'll wait a bit.
< wumpus>
I think we should comment out the whole function until someone sorts out the problem
< wumpus>
then when brining it back we can make the decision, should it crash or just report
< wumpus>
but that's after we've fixed the non-sensical reports
< jonasschnelli>
Agree
< wumpus>
even having it as special autoconf option is not useful as long as it misfires
< jonasschnelli>
Implementation details and things there where to much "Core-only" for bitcoin-dev list can now be discussed on bitcoin-core-dev
< jonasschnelli>
why the heck does "sendtoaddress" has a "comment" and a "comment-to" argument? Isn't this very confusing?
< Luke-Jr>
it's all confusing
< wumpus>
yes
< wumpus>
axe them
< Luke-Jr>
should really just be one comment/label per address. simple and clean.
< sipa>
it stores a comment on the address and a comment on the transaction, afaik :)
< jonasschnelli>
hmm... I think we should only have comments for transactions.
< jonasschnelli>
Doesn't labels for addresses as well as a "address book" implies address-resuse?
< Luke-Jr>
jonasschnelli: addresses are transactions; the difference is an address may not have a blockchain-transaction yet (ie, it might just be a request)
< wumpus>
no, not really, remember multiple addresses can have the same label jonasschnelli
< wumpus>
label is just a way to group addresses
< jonasschnelli>
wumpus: Ah. Right. That makes more sense.
< wumpus>
'all addresses I've sent to that belong to jonasschnelli' etc
< wumpus>
'all addresses Ive used to receive from XYZ'
< jonasschnelli>
So a send-command then requires a comment (wtx) and a label (for the to address)
< jonasschnelli>
What about a label for the change-address? Not necessary i guess
< wumpus>
in any case, please make the command accept a structure
< wumpus>
so you can have optinoal arguments as well as extensibility
< Luke-Jr>
jonasschnelli: I don't see value in association with a wtx rather than an address.
< wumpus>
not the current positional hellscape
< jonasschnelli>
wumpus: Yes. What do you think in making the whole parameters for the new wallet an assoc. array? (A JSON object).
< wumpus>
we don't have to lock this down now if the API is extensible
< wumpus>
yes please
< jonasschnelli>
So each parameter has always a key
< wumpus>
right
< jonasschnelli>
Okay... and maybe also auto-detecting JSON object in bitcoin-cli to get rid of the static conversion table.
< wumpus>
yes I think a RPC call to get the command line to JSON conversion table would make sense
< wumpus>
although OTOH it's not a server concern, it's more like 'give me machine parsable docuentation'
< jonasschnelli>
Okay.
< wumpus>
(I've forgotten the exact name for this, but I think there's even an issue open about it, about automatic discoverability of the interfaces etc)
< sipa>
i remember something like that
< sipa>
even some existing standard
< wumpus>
right
< wumpus>
'introspection' is the word
< jonasschnelli>
But isn't the JSON conversion table something 100% on the client side?
< wumpus>
jonasschnelli: well in a way it is, but other applciations (such as a nice ncurses based interactive console) may want the same information
< jonasschnelli>
ah. Something like a WSDL
< wumpus>
basically to help the client know what fields are expected, what types, etc, without having to hardcode this
< wumpus>
"The only drawback that I see is that bitcoin-cli (and other RPC command-line clients) would effectively have to do two roundtrips to the server. Once to get instructions on how to convert the command-line parameters to JSON, and once to do the actual command." apparently I forgot about a thing called caching :p
< jonasschnelli>
wumpus: we could response a static JSON file that contains the "introspect". So people could store this on the client side.
< wumpus>
exactly
< jonasschnelli>
Like a SOAP WSDL
< jonasschnelli>
Also, I'm not sure if a rpc-command prefix instead of a URL endpoint is more appropriate. At least the first is way more simple to implement.
< wumpus>
mostly-static, it can change based on server configuration (different wallets, no wallet, etc)
< jonasschnelli>
Agree
< GitHub117>
[bitcoin] fanquake opened pull request #7838: [Doc] Update gitian build guide to debian 8.4.0 (master...gitian-debian-84) https://github.com/bitcoin/bitcoin/pull/7838
< sipa>
whenever you trigger a rpc type conversion error
< jonasschnelli>
We could use /wallet for the new wallet commands... or wallet_command at the same root (/) endpoint
< jonasschnelli>
sipa: +1
< sipa>
you get the new table
< wumpus>
or add an optional header to the RPC request: Fail-if-conversion-table-hash-is-not: XXXXX
< wumpus>
then on failure retrieve the new list and do the conversion and request again
< wumpus>
jonasschnelli: yes, different endpoints is also possible, we've discussed this in the path for multi-wallet support
< wumpus>
in the PAST
< wumpus>
you'd have to rewrite the RPC server a bit though so you instance it multiple times
< jonasschnelli>
Yes. Command prefixes would be much simpler.
< jonasschnelli>
Also the Multiwallet would be trivial with RPC argument structures.
< jonasschnelli>
Just pass a {"wallet" : "wallet1", ... }
< wumpus>
you mean command prefixes such as 'wallet.getinfo' 'mempool.getinfo' etc
< jonasschnelli>
If no wallet is defined, use the "default" wallet
< wumpus>
that won't help multiple wallets of the same type, of course
< wumpus>
I wouldn't like wallet_wumpus.getinfo etc
< wumpus>
right
< sipa>
use separate url for each wallet
< * jonasschnelli>
likes the <dot>. syntax.
< jonasschnelli>
url endpoints is non-trivial to implement.
< wumpus>
does JSONRPC even allow dots in method names?
< wumpus>
it's pretty trivial to implement jonasschnelli
< jonasschnelli>
hmm.. good question. :)
< sipa>
jonasschnelli: it means that different wallets with different api can be loaded simultaneously
< wumpus>
we've done quite some work in making that possible already
< jonasschnelli>
okay... need to look at this.
< wumpus>
just means we need some refactors which we really want anyway
< jonasschnelli>
sipa: can you explain: "different wallets with different api can be loaded simultaneously"?
< wumpus>
yes, if they have different endpoints, they can co-exist without even noticiing each other
< wumpus>
(assuming they won't touch the same files etc)
< sipa>
jonasschnelli: say we have a schnelliwallet, and then later someone implement a super simple sipwallet with a different api, using the same rpc table for both won't work
< jonasschnelli>
something like /wallet/mywallet/getinfo and wallet/createnewwallet
< sipa>
jonasschnelli: if they run on different urls, you get that for free
< wumpus>
in any case, let's not do this all at once, the new wallet explicitly doesn't have a stable interface in the beginning
< jonasschnelli>
okay.
< jonasschnelli>
A /wallet endpoint makes sense for now I guess.
< jonasschnelli>
Also for further process de-coupeling.
< wumpus>
people will already instance multiple RPC proxies so whether that points to two URLs on the same server or different ports is a detail
< wumpus>
gmaxwell: hahahahah oops
< jonasschnelli>
Hm... now for the "bumpfee" RBF feature,.. do I really need to add a 6. argument to "sendtoaddress"? I can't see a better option to opt-into-RBF.
< gmaxwell>
start adding data to the address field with delimiters that can't be in addresses.
< * gmaxwell>
ducks
< jonasschnelli>
gmaxwell: almost as good as "sendtoaddress-with-rfb" :-)
< gmaxwell>
incidentally when comming up with new wallet-apis, it really should be possible to do things like "send all my funds, minus whatever fees are needed" or even "send at least x, but you can send a bit more to avoid creating change".
< wumpus>
or just encode all arguments into the address with a special prefix
< gmaxwell>
wumpus: we have that api already, it's called 'sendrawtransaction' :)
< wumpus>
gmaxwell: this is for the old wallet API, for the new one the argument will be a structure so there's flexibility and expansibility, for the old one we're stuck with positional argument madness
< jonasschnelli>
gmaxwell: "send all my funds, minus whatever fees are needed" is think this is already possible npw
< wumpus>
it's just that you don't want to add even more boolean arguments, like one for 'you can send a bit more to avoid creating change'
< wumpus>
which should be possible with a better API
< jonasschnelli>
wumpus: hah. True. I saw many people trying to move their balance from one to another wallet by slowly picking the total amount (like a in-human-fee-estimator)
< wumpus>
haha yes I think I've done that once too
< wumpus>
bisecting the number
< gmaxwell>
oh when did that get added! I missed that.
< gmaxwell>
that bisect behavior also then results in some stupid tiny amount being left in the wallet often.
< gmaxwell>
but yes, it was just an aside comment for new APIs.
< GitHub35>
bitcoin/0.12 465d309 Wladimir J. van der Laan: doc: update release notes for #7835
< wumpus>
* [new tag] v0.12.1rc2 -> v0.12.1rc2
< michagogo>
wumpus: script running, should push sigs shortly
< wumpus>
thanks!
< michagogo>
If the previous PR is still around it'll just update that (and confuse the script), otherwise there'll be a new one
< MarcoFalke>
Has anyone run the test suite on 0.12 yet?
< michagogo>
(That last part isn't really a problem-- even though it runs in bash -e, it's the last line of the script IIRC so it shouldn't hurt anything)
< GitHub17>
[bitcoin] dragongem45 opened pull request #7839: Expose information on whether transaction relayed is enabled in getne… (master...patch-2) https://github.com/bitcoin/bitcoin/pull/7839
< MarcoFalke>
Is there a difference between `int(unsigned(1))` and `static_cast<int>(unsigned(1))`?
< sipa>
MarcoFalke: no, both will return the same int
< sipa>
MarcoFalke: the first is weird C, the second is weird C++ :)
< MarcoFalke>
but they are different from `(int) ((unsigned) (1))`?
< MarcoFalke>
Assuming int and unsigned are any primitive type.
< sipa>
TYPE(VAL) is only valid in C++
< sipa>
in C you have to use (TYPE)VAL
< sipa>
int(5) is technically invoking the C++ constructor for int, with argument 5
< sipa>
(int)5 is a C cast from 5 to int
< sipa>
static_cast<int>(5) is a C++ cast from 5 to int
< wumpus>
TYPE(VAL) syntax also doesn't work with e.g. pointer types, and other types that aren't one word because they have specifiers (e.g. unsigned int)
< MarcoFalke>
You can put (unsigned int) into braces and it should work, I think
< wumpus>
yes but that's just a C cast then
< MarcoFalke>
Ok, I was wondering what is preferred. E.g. static_cast<int>(nSize) or int(nSize)
< MarcoFalke>
re: negative fee rates
< sipa>
the first
< MarcoFalke>
so it's clear that a cast is happening?
< sipa>
the latter is C style, and its behaviour is a bit unpredictable
< wumpus>
I don't really care, more important when casting between int types is to handle overflows and underflows properly ,none of the C/C++ casts does that in itself
< wumpus>
but yes in C++ using a C++ cast is probably best
< wumpus>
isn't int(X) c++ syntax too, though?
< wumpus>
in C, int doesn't have a constructor
< sipa>
int(X) is C++, yes
< wumpus>
you could only do (int)X
< sipa>
but it's not a cast :)
< sipa>
... semantics, though
< wumpus>
what is the difference in practice?
< wumpus>
(for int, for other objects it probably invokes different methods)
< sipa>
ok, i learned today:
< sipa>
The functional cast expression consists of a simple type specifier or a typedef specifier (in other words, a single-word type name: unsigned int(expression) or int*(expression) are not valid), followed by a single expression in parentheses. This cast expression is exactly equivalent to the corresponding C-style cast expression.
< wumpus>
ah, that makes sense
< sipa>
so it's just extra C++ syntax equivalent to a C cast, with the same magic behaviour
< wumpus>
I don't understand this line (target-bin/upgrade-system.sh in gitian): DEBIAN_FRONTEND=noninteractive apt-get -y dist-upgrade > /dev/null > /var/cache/gitian/upgrade.log 2>&1
< wumpus>
what is sent to /dev/null and what to the log file?
< wumpus>
(my guess is everything goes to /dev/null and the second > is ignored, but I just don't know the syntax)
< helo>
everything ends up in /var/cache/gitian/upgrade.log (out and err)
< wumpus>
then what does the /dev/null do?
< sipa>
wumpus: i believe i know the syntax, and that >/dev/null is redundant
< MarcoFalke>
wumpus, do you really want to assert(int(nSize)>0)? nSize must represent a size of some GB (~ 1e9 GB, I think)...
< wumpus>
should be >=0 I think?
< GitHub164>
[bitcoin] sipa opened pull request #7840: Split and optimize inv queues and improve mempool privacy (master...splitinvtxblock) https://github.com/bitcoin/bitcoin/pull/7840
< wumpus>
but yes, such an assertion makes sense, to make sure the number is within the right range
< wumpus>
I don't think it'll ever trigger but better be safe than sorry
< MarcoFalke>
You don't want to have some code to be triggered by p2p which will cause the assert to fail ...
< MarcoFalke>
And remotly shut down any node
< wumpus>
that may be preferrable to the alternative, whatever happens with the big negative fee
< MarcoFalke>
if(nSize<0)nSize=max_nr
< MarcoFalke>
what about this?
< wumpus>
I don't like that, if somethat that wrong happens it's better to just stop, ignoring issues is worse than simply handling them
< MarcoFalke>
ok
< wumpus>
it's a bug in our code if a size of ~1e9 GB reaches the fee rate function
< morcos>
wumpus: are you sure that should be the case (that it's a bug?). ok i guess 1e9 GB, maybe makes sense.. but you could make the argument that someone would use CFeeRate to report the average fee rate of the whole mempool or something.
< wumpus>
well it overflows the integer range
< wumpus>
how would you handle that?
< morcos>
In other words, i think maybe its ok to have some reasonable limit, but maybe we should clearly flag that rather than just making assumptions on how CFeeRate is used..
< helo>
should data type validity be ensured during deserialization?
< morcos>
I guess i just don't have a good mental model of where/when CFeeRate is used. So I agree good for it to be a bug when it overflows, but lets make sure that limit is high enough and comment the limitations well in CFeeRate. 1e9GB seems fine to me
< wumpus>
I'm tired of this discussion already, I personally feel bad about silent c++ signed/unsigned casts and the potential overflow scenarios, but feel free to just ignore the issue for CFeerate.
< MarcoFalke>
Will add the assert and the doc
< MarcoFalke>
Hopefully everyone is happy then
< wumpus>
as fee rates are not used to address into arrays this will probably never result in exploitable scenarios or memory crashes
< morcos>
Yeah all I'm trying to suggest is that we make our assumptions explicit, sorry I probably didn't state that clearly
< morcos>
so sounds good
< wumpus>
the point is that a size_t can be larger than std::numeric_limits<int64_t>::max() on most platforms
< sipa>
use ptrdiff_t *ducks*
< wumpus>
so before the cast it makes sense to check that the value is <= that
< wumpus>
what you do in that case depends on the circumstances, but in this case as it shoudl be a bug if such a big value reaches it an assertion makes sense, I'd think
< wumpus>
sipa: right, that would just move the issue to the interface :)
< wumpus>
so if you say "it should be able to handle the entire mempool", I still think it would be an abomination if the mempool was larger than half the virtual address space on 64-bit platforms :)
< GitHub19>
[bitcoin] dragongem45 closed pull request #7839: Expose information on whether transaction relayed is enabled in getne… (master...patch-2) https://github.com/bitcoin/bitcoin/pull/7839
< wumpus>
helo: probably, as the deserializer has knowledge of the types involved
< wumpus>
if it doesn't check type validity, then who will? having deserialized objects in memory constructed with invalid bit patterns could be a security or stability issue
< wumpus>
shit, just wiped the gitian output again by starting before I copied out the executables
< GitHub24>
[bitcoin] dragongem45 opened pull request #7841: Expose information on whether transaction relayed is enabled in getnetwork (master...patch-2) https://github.com/bitcoin/bitcoin/pull/7841
< morcos>
sdaftuar: wumpus: i got a failure of bip68-sequence.py in 0.12.1rc2 . but its intermittant. looking into it.
< morcos>
i suspect the problem is that if the version of your bitcoind that generated your cache is old then the CSV activation doesn't happen as expected in that test
< * MarcoFalke>
Should buy more ram so gitian can run while working
< morcos>
yes thats the problem, i was confused b/c there are two cache directories. one in pull-tester and one in rpc-tests
< morcos>
would it make sense for make clean to wipe out the cache directories?
< morcos>
jonasschnelli: oops forgot to ping you. i'm here.
< wumpus>
morcos: so removing the cache made the problem go away?
< morcos>
wumpus: yep. it works fine with a fresh cache.
< morcos>
it makes sense that the old cache wouldn't work
< wumpus>
phew!
< btcdrak>
why are gitian builds so slow in comparison to building on a non VM using same number of cores to compile with?
< MarcoFalke>
depends take the longest time
< sdaftuar>
morcos: oops, thanks for figuring it out. seems innocuous enough to not worry about fixing in 0.12.1?
< GitHub122>
[bitcoin] paveljanik opened pull request #7842: RPC: do not print minping time in getpeerinfo when no ping received yet (master...20160408_getpeerinfo_no_ping_yet) https://github.com/bitcoin/bitcoin/pull/7842
< instagibbs>
during a reorg, when(if ever) does a node re-broadcast transactions that have re-entered the mempool
< morcos>
instagibbs: i assume it does not
< instagibbs>
matches up with my testing just making sure
< instagibbs>
python testing kind of breaks when that happens, since many times it checks to ensure that mempools are synced
< btcdrak>
interesting. I just noticed Github has started verifying GPG signed commits if you give it your key https://i.imgur.com/MNIYMm0.png
< Chris_Stewart_5>
Does the 'bool' returned by EvalScript inside of interpreter indicate that the transaction has been marked invalid, or that the script execution ended in the returned bool value?'
< sipa>
EvalScript just evaluates, it doesn't check success conditions
< sipa>
so if it returns false, it means an error occurred
< sipa>
VerifyScript tests the success conditions for use in transactions
< Chris_Stewart_5>
because shouldn't the top of the stack indicate if the script exectuion was true/false?
< sipa>
yes, VerifyScript tests that
< Chris_Stewart_5>
gotcha. Thanks for the clarification
< kanzure>
for emails rejected on the bitcoin-core-dev mailing list, should they also be forwarded to bitcoin-dev-moderation@lists.ozlabs.org or should that feed remain for bitcoin-dev mailing list rejection only?