< meshcollider>
Ok but just this once ;) I pushed the fix for your last nit for the iswitness decode parameter btw
< sipa>
awesome, thanks
< meshcollider>
I'm sitting on a bus for 8 hours today across the country, anything would be useful for me to work on while I'm sitting here? Otherwise I'll just go and review some more PRs
< jjackson502>
join
< meshcollider>
Is it better to keep src/test/testutil.cpp|h for future use even if they're kinda redundant, or better to remove them because it results in fewer source files?
< sipa>
i don't think anyone cares about the number of source files, as long as the separation in files makes sense (don't make a new file for eveey function...)
< jimpo>
The stall timeout for headers sync (nHeadersSyncTimeout) appears to be a single timeout to sync the whole header chain instead of a smaller timeout for each successive headers message. Why is that?
< midnightmagic>
+1 no file-per-function nonsense. not everyone uses a class-browser and ignores other structure.
< gmaxwell>
jimpo: it's not very sophicated right now. I'm sure it could also have several tiers of timeouts, one thing to be warry of is the sum of small timeouts if fed slowly turning into an unreasonably long wait.
< meshcollider>
midnightmagic: I'm suggesting removing a file with only one function (a redundant function which just calls another), not adding new files
< * midnightmagic>
shrugs
< gmaxwell>
meshcollider: if you don't know you can also feel free to just make a PR to remove it per your best judgement and if you were mistaken people will explain why. No big deal.
< kallewoof>
sipa: Slightly off topic, but a new file for every public class would be sort of nice though (net.h has 4, for example).
< sipa>
kallewoof: i couldn't disagree more, soory:)
< kallewoof>
sipa: If they're small, I can live with it, but net.cpp is 3k lines. I think that's a bit long.
< sipa>
oh, absolutely
< sipa>
but nrt.cpp is also way more than one class
< kallewoof>
CConnman is about 2k, the rest is CNetMessage and CNode and random help functions. Not sure what you mean by 'more than one class' -- that's what I'm saying too, it's 3 classes.
< kallewoof>
Anyway, it was a random thought. I like files to be less than 1k lines if possible. Maybe that's just a personal preference.
< sipa>
kallewoof: what i mean is that in an ideal world a lot of the functionality in net could be split up into more classes, but i think many of them may still belong in net
< sipa>
i don't see a 3k line file as a big problem, but it is suboptimal
< sipa>
but splitting up every class into its own file is throwing away the ability for files to convey a structure themselves
< kallewoof>
That makes sense, yeah. I guess my erk is really with file size, not classes in the same file..
< sipa>
right
< sipa>
file structure is there to make you help find things
< sipa>
if you know the name of every class in the project, and how they relate, then i guess one class per file is optimal
< sipa>
but typically you don't, and if when investigating program flow you need to jump between files all the time, that's also not very helpful
< kallewoof>
It feels like a lot of file jumping happens right now though. Everything and its aunt seems to at some point go into validation.cpp, for example. I really wish that file was at least 2 or 3 files with distinct purposes.
< kallewoof>
I know it's been improved though. (I think it was 7k+ lines when I started and it was still called main.cpp)
< sipa>
kallewoof: right, the key point is "with distinct purposes"
< * kallewoof>
nods.
< promag>
> when investigating program flow you need to jump between files all the time, that's also not very helpful
< promag>
sipa: the same for big files?
< Victorsueca>
did anyone check my question about Windows GUI fee setting?
< meshcollider>
Victorsueca: sorry what question? Link?
< Victorsueca>
meshcollider: when setting the fee on Windows with the gui it only displayed the estimate for 2 blocks
< Victorsueca>
I was going to ask if it was intended, but seems it's just it was creating fee estimates
< Victorsueca>
whatever setting I used would only display "Estimated to begin confirmation within 2 blocks"
< Victorsueca>
now it's been up for several days and it display up to 150 blocks
< meshcollider>
So it's working as expected now?
< meshcollider>
Btw Travis needs rerunning on #11062 for someone with git superpowers :)
< Victorsueca>
I have no idea how is it supposed to work, but seems usable to me
< jnewbery>
trippysalmon : that's a really weird failure. The test framework is failing to read a username:password from a file, even though it previously read them successfully. I think this must be some random Travis failure. I suggest you ask jtimon to restart the Travis job (since it's a PR to his repo)
< sipa>
i can restart jobs
< trippysalmon>
jnewbery: thanks, i thought something like that might be the case, haven't looked to deep into it after i found that out
< trippysalmon>
sipa: that would be great
< sipa>
which PR, sorry?
< trippysalmon>
it's PR #9, but it's on jtimon's repo
< sipa>
oh!
< sipa>
then i can't
< trippysalmon>
aww, thanks anyway, i'll ask jtimon when i see him around
< PRab>
FYI, I did a gitian build of 0.15.0rc3, installed it (Windows 10 64 bit), had it upgrade my UTXO db (took ~15 minutes), and all appears to be running well! Keep up the great work!!
< sipa>
PRab: cool
< jnewbery>
trippysalmon : alternatively, you can `git commit --amend` to update the timestamp on your most recent commit and then `git push -f`. New timestamp => new SHA => Travis will rerun.
< trippysalmon>
jnewbery: clever man :) will do that, thanks
< MarcoFalke>
tripy
< MarcoFalke>
trippysalmon: The bug was fixed in current master
< MarcoFalke>
The issue was that the cookie file was not atomically written on creation
< trippysalmon>
MarcoFalke: ah ok, thanks for letting me know. this is my first time working on the core codebase so i was unsure if it was my mistake or not
< trippysalmon>
ill leave the code as it is then and assume it will work out
< MarcoFalke>
Jup, rerunning should work around the issue
< trippysalmon>
it's currently running so i hope you're right
< trippysalmon>
okay all jobs finished successfully, thanks all
< BlueMatt>
note: after #10387 (or some way to identify a node as full but pruned) we should stop accepting requests to fast-relay compact blocks pre-validation from non-full nodes. Its a drastic reduction in their security model and unlikely to be worth the tiny change in relay time. I'm especially concerned that some of the spv clients are going to implement compact blocks and then someone who had set up bitcoin core as a border/firewall node
< BlueMatt>
between their spv wallet and the world will suddenly have only spv security instead of full security without them even noticing
< bitcoin-git>
[bitcoin] practicalswift closed pull request #10972: [Qt] Check return value of addr.GetKeyID(keyid) on custom change address change (master...GetKeyID-assertion) https://github.com/bitcoin/bitcoin/pull/10972
< promag>
should be foo(const CAmount& amount) or foo(CAmount amount) ?
< BlueMatt>
I think we use both...cause CAmount is secretly just int64_t I'm not sure it matters, but there was some idea of making CAmount assert that you dont overflow at some point in the future