1 2017-01-13 00:31:27	0|BlueMatt|cfields: want to review #9535 as well?
  2 2017-01-13 00:31:28	0|gribble|https://github.com/bitcoin/bitcoin/issues/9535 | Split CNode::cs_vSend: message processing and message sending by TheBlueMatt · Pull Request #9535 · bitcoin/bitcoin · GitHub
  3 2017-01-13 00:44:31	0|jtimon|#9535 seems easy to review for those familiar with the network code
  4 2017-01-13 00:44:33	0|gribble|https://github.com/bitcoin/bitcoin/issues/9535 | Split CNode::cs_vSend: message processing and message sending by TheBlueMatt · Pull Request #9535 · bitcoin/bitcoin · GitHub
  5 2017-01-13 00:44:52	0|BlueMatt|(its also a big performance bump)
  6 2017-01-13 00:45:06	0|BlueMatt|or, well, easy to review for anyone who can read a list of variable names and trace their usage :p
  7 2017-01-13 00:47:55	0|jtimon|yeah, I wish I could review that, but never paid much attention to the network code above processMessages and I wasn't able to keep up with the recent changes (I still greatly appreciate the separation and look forward studying it better after all these improvements)
  8 2017-01-13 00:49:46	0|BlueMatt|well #9535 is just a lock split, so just going through each variable that is accessed in one side and making sure its not accessed on the other is a pretty good (though admittedly not 100% sufficient) review
  9 2017-01-13 00:49:47	0|gribble|https://github.com/bitcoin/bitcoin/issues/9535 | Split CNode::cs_vSend: message processing and message sending by TheBlueMatt · Pull Request #9535 · bitcoin/bitcoin · GitHub
 10 2017-01-13 00:50:01	0|BlueMatt|and one one of the sides in this case there are relatively few variables accessed, so its not so hard
 11 2017-01-13 00:50:21	0|jtimon|I just meant the code changes are small
 12 2017-01-13 00:52:24	0|jtimon|I guess I could do a mechanical review like what you propose
 13 2017-01-13 00:52:53	0|cfields|BlueMatt: yep, will do after dinner
 14 2017-01-13 00:55:00	0|cfields|jtimon: yea, it's sane, just needs manual review of what's accessed under the locks before/after
 15 2017-01-13 00:59:30	0|cfields|(note that you can treat it as non-recursive, so very simple)
 16 2017-01-13 00:59:46	0|jtimon|yeah, I considered it as the network-related PR I was more likely to be able to review, but then I tried to estimate how much would it take to do that and my answer was "no idea, let's move to another PR for now", if you say it's easy, thanks, I'll gladly do that (maybe not today, was hoping to at least utACK some individual commits in #9512)
 17 2017-01-13 00:59:48	0|gribble|https://github.com/bitcoin/bitcoin/issues/9512 | Fix various things -fsanitize complains about by sipa · Pull Request #9512 · bitcoin/bitcoin · GitHub
 18 2017-01-13 01:01:20	0|cfields|jtimon: np. no obligation, was just echoing what matt said
 19 2017-01-13 01:07:00	0|jtimon|cfields: sure, it's a good tip, if you guys tell me it shouldn't take long to do that manual review I think it's a good use of my time, hey, it's dividing an important lock into 2! I guess I cs_main got me too scared about reviewing concurrency unless it was cs_args, cs_mempool (encapsulated in its class), or maybe even a 10-line singleton for libsecp's context or something
 20 2017-01-13 01:08:00	0|jtimon|will review #9535
 21 2017-01-13 01:08:01	0|gribble|https://github.com/bitcoin/bitcoin/issues/9535 | Split CNode::cs_vSend: message processing and message sending by TheBlueMatt · Pull Request #9535 · bitcoin/bitcoin · GitHub
 22 2017-01-13 02:14:54	0|jtimon|in case someone gets bored among so many things to review somehow...#8855 has been needing review for so long...
 23 2017-01-13 02:14:56	0|gribble|https://github.com/bitcoin/bitcoin/issues/8855 | Use a proper factory for creating chainparams by jtimon · Pull Request #8855 · bitcoin/bitcoin · GitHub
 24 2017-01-13 02:18:44	0|jtimon|about a previous version of it gmaxwell once said "This adds news without frees", and I believe I removed all the non-frees many rebases ago...
 25 2017-01-13 02:23:15	0|jtimon|also, how should I benchmark #8498 to make it attractive?
 26 2017-01-13 02:23:16	0|gribble|https://github.com/bitcoin/bitcoin/issues/8498 | Optimization: Minimize the number of times it is checked that no money... by jtimon · Pull Request #8498 · bitcoin/bitcoin · GitHub
 27 2017-01-13 02:24:17	0|BlueMatt|jtimon: do a few reindexes with and without it on a relatively unused machine
 28 2017-01-13 02:29:05	0|jtimon|BlueMatt: that will show some improvement (since connectblock is still calculating the tx fee twice), but the most interesting effect should be with acceptToMemoryPool...oh,wait, now that's only calculating it twice per tx too (it used to be much more), yeah, thanks, can try that (probably after 0.14)
 29 2017-01-13 02:32:07	0|jtimon|should probably have a look at src/bench and maybe try to do the reindex from there
 30 2017-01-13 02:36:18	0|BlueMatt|luke-jr: re: FlexVer...talos failed to get the (crazy high) funding they were trying to raise, so thats not gonna happen
 31 2017-01-13 02:36:30	0|luke-jr|BlueMatt: no, FlexVer survives beyond Talos
 32 2017-01-13 02:36:36	0|luke-jr|without more funding
 33 2017-01-13 02:36:39	0|BlueMatt|oh? are they putting it in something else?
 34 2017-01-13 02:37:07	0|BlueMatt|(my understanding was the raptor folks were essentially saying "ok, well we didnt get the funding, so if you want any pieces of talos you have to come pay us for them")
 35 2017-01-13 02:37:11	0|luke-jr|non-workstation/servers
 36 2017-01-13 02:37:45	0|BlueMatt|well, ok, technically they have another 48 hours to raise the missing 3.something million USD.....
 37 2017-01-13 02:38:39	0|luke-jr|FlexVer-based VPS is their fallback after Talos failing
 38 2017-01-13 02:38:45	0|midnightmagic|that funding target was pretty nuts. :-(
 39 2017-01-13 02:38:49	0|BlueMatt|oh? link?
 40 2017-01-13 02:38:56	0|BlueMatt|midnightmagic: yea...real shame, too
 41 2017-01-13 02:39:24	0|BlueMatt|they should have done the shit the ORWL folks did - raise a modest amount and assume you can sell more of the hardware later to make up the missing costs
 42 2017-01-13 02:39:35	0|BlueMatt|of course its a big risk, but at least we'd get cool stuff out of it :)
 43 2017-01-13 02:39:42	0|midnightmagic|Perhaps they just didn't have the money to do it to begin with.
 44 2017-01-13 02:39:56	0|luke-jr|BlueMatt: #talos-workstation discussion
 45 2017-01-13 02:39:58	0|BlueMatt|luke-jr: oh? link?
 46 2017-01-13 02:40:07	0|midnightmagic|Similar failures have happened many times in the past e.g. phase5's attempt to resurrect the Amiga with the A\Box
 47 2017-01-13 02:40:09	0|BlueMatt|ooo, didnt know about that
 48 2017-01-13 03:03:41	0|gmaxwell|BlueMatt: I dunno, maybe, will have to think; but if you're concerned about security here there are many better things to do that would harden it that wouldn't change performance.
 49 2017-01-13 03:04:04	0|gmaxwell|BlueMatt: e.g. make it so that it can't skip signatures in a reorg.
 50 2017-01-13 03:04:28	0|cfields|BlueMatt: thanks for fixing up the const. I think my gripe wasn't really justified. The const rules for pointers/references still trip me up sometimes.
 51 2017-01-13 03:04:40	0|BlueMatt|gmaxwell: hadnt thought about that one...came up with a few things which addressed the specific scenario I described above, though figured that would be very scenario-specific :/
 52 2017-01-13 03:05:12	0|BlueMatt|cfields: I agree it was super hard to read what was going on there, and why it wasnt some crazy const_cast unsafe bullshit the way it read
 53 2017-01-13 03:05:41	0|gmaxwell|BlueMatt: another one is to not skip signatures when there is a long fork that doesn't cointain this block.
 54 2017-01-13 03:06:12	0|gmaxwell|(both of these were security features I recommended for the hashpower only decision-- but I didn't think their complexity was worthwhile with a static hash.)
 55 2017-01-13 03:06:14	0|cfields|BlueMatt: right, that. const_cast usually just sets off alarms to me that something's fishy. The change makes it easy to reason about.
 56 2017-01-13 03:06:19	0|BlueMatt|gmaxwell: yea, I wanted to avoid suggesting adding twenty ways it could be hardened and risk it slipping for 0.14
 57 2017-01-13 03:07:14	0|gmaxwell|I think both are more powerful than just burriedness, I even only did the burriedness because it was ~1 LOC.
 58 2017-01-13 03:09:58	0|BlueMatt|gmaxwell: both assume you actually see the other branch, which isnt a crazy assumption, but is different from burriedness
 59 2017-01-13 03:10:07	0|BlueMatt|well, reorg less so
 60 2017-01-13 03:10:15	0|BlueMatt|but either way, I'm happy if 2 weeks meets your performance goals
 61 2017-01-13 03:10:41	0|BlueMatt|if we come up with fun ways to strengthen it in the future (eg when jl2012's fork warning stuff gets fixed it would be easy to combine that), then we can do that
 62 2017-01-13 03:10:45	0|luke-jr|sipa: essentially DRM that allows selling encrypted VPSs that the physical-access/datacentre/host-manager cannot see into or control (beyond poweroff/delete)
 63 2017-01-13 03:11:43	0|cfields|jonasschnelli: I'm going through #9294, though I'm afraid I don't know the code well enough to give any helpful review. So nits is the best I can do :(
 64 2017-01-13 03:11:45	0|gribble|https://github.com/bitcoin/bitcoin/issues/9294 | Use internal HD chain for change outputs (hd split) by jonasschnelli · Pull Request #9294 · bitcoin/bitcoin · GitHub
 65 2017-01-13 03:12:10	0|gmaxwell|BlueMatt: the forkwarning stuff 'brokenness' actually wouldn't effect that.
 66 2017-01-13 03:13:00	0|gmaxwell|because in that case you'd be on the attack chain, which is best header, but there would be a long valid fork that doesn't contain your block.
 67 2017-01-13 03:17:19	0|cfields|sipa: am I going to throw you off if I go ahead and squash 9441? Not sure if you want to have another look after the last few cleanups
 68 2017-01-13 03:17:43	0|BlueMatt|gmaxwell: ok, so do you want to add that? I do not think it is a replacement for setting the time to 2 weeks, plus setting the time to 2 weeks avoids the need to get re-reviews when we're already not gonna make our goals
 69 2017-01-13 03:17:46	0|gmaxwell|14:52 < jtimon> regarding the 5.5 vs 7.5 h benchmark, that's only for fresh releases or people setting the arg manually right before the  limit, right?
 70 2017-01-13 03:17:49	0|gmaxwell|14:52 < BlueMatt> yes
 71 2017-01-13 03:17:57	0|gmaxwell|no increasing the offset increases the total sync time for that software forever.
 72 2017-01-13 03:18:21	0|BlueMatt|huh?
 73 2017-01-13 03:18:28	0|BlueMatt|did i misread the code?
 74 2017-01-13 03:19:35	0|gmaxwell|oh no I'm being momentarily stupid.
 75 2017-01-13 03:19:38	0|BlueMatt|the software is updated with some assumevalid....at that time all blocks a week before that assumevalid arent validated...one week later all blocks up to that assumevalid are validated
 76 2017-01-13 03:19:43	0|BlueMatt|ok, phew
 77 2017-01-13 03:19:44	0|jtimon|huh? +1 (I bet the answer is in GetBlockProofEquivalentTime which is the part I didn't fully read because it wasn't being changed in that PR)
 78 2017-01-13 03:20:09	0|gmaxwell|BlueMatt: I'm referring to the case where the user updates it. Which I expect people to do on low power devices.
 79 2017-01-13 03:20:27	0|BlueMatt|ahh, yes, ok
 80 2017-01-13 03:21:01	0|gmaxwell|it basically says you can't sync without processing N-back no matter what you set. So if you're on a low power device where a month of blocks takes hours, thats what you're left with.
 81 2017-01-13 03:22:20	0|BlueMatt|how long does it take to sync a week's blocks on an rpi?
 82 2017-01-13 03:22:33	0|BlueMatt|or, cdecker you were complaining about stuff on the cheapest digitalocean thing
 83 2017-01-13 03:22:36	0|BlueMatt|how slow was that?
 84 2017-01-13 03:23:23	0|jtimon|but the user doesn't have to do anything right? just wait for the program to fully  the N last blocks, right?
 85 2017-01-13 03:23:59	0|gmaxwell|BlueMatt: a lot of people just copy the chain from another system; instead they could copy the assume block.
 86 2017-01-13 03:23:59	0|jtimon|s/fully  the/fully verify the/
 87 2017-01-13 03:24:47	0|BlueMatt|on the flip side, a lot of setup guides will tell you to copy blockchain.info's latest hash as the assumeblock the day after release :p
 88 2017-01-13 03:25:16	0|gmaxwell|I also worry that making this too slow increases the occurance of people copying the state. E.g. there is someone on reddit that responds to every thread about slow sync with a download for a synced node.
 89 2017-01-13 03:25:34	0|BlueMatt|oh, agreed, but there is a line to walk there
 90 2017-01-13 03:25:51	0|gmaxwell|BlueMatt: thats better than taking a whole download of the state which people are already being told to do.
 91 2017-01-13 03:26:00	0|jtimon|re N blocks vs time: I definitely like height more, for everything related to time in consensus
 92 2017-01-13 03:26:02	0|BlueMatt|(I'd tend to agree that a week is sufficient to keep stupidity of random block explorers from creeping into consensus)
 93 2017-01-13 03:26:19	0|BlueMatt|just prefer > 1 week for other reasons
 94 2017-01-13 03:26:43	0|BlueMatt|I'm open to being convinced if it really is prohibitively slow (compared to the rest of chainsync w/o sig-checks)
 95 2017-01-13 03:28:27	0|jtimon|To reiterate, I don't see the danger with doing a month either: advanced users that look for top performance can compile the code changing the month to a week or 2 blocks
 96 2017-01-13 03:28:46	0|BlueMatt|I dont think thats a realistic request
 97 2017-01-13 03:32:38	0|gmaxwell|jtimon: none of this should involve time or height.
 98 2017-01-13 03:32:47	0|gmaxwell|__sigh__ both are trivially controlled by miners.
 99 2017-01-13 03:32:54	0|gmaxwell|the test in the code involves work.
100 2017-01-13 03:33:15	0|jtimon|I guess my point is that "I want to resync faster than the assumevalid by default in the last release" use case is not very compeling
101 2017-01-13 03:33:39	0|gmaxwell|jtimon: it is very compelling when it's taking days to sync and can be fixed.
102 2017-01-13 03:34:51	0|jtimon|gmaxwell: the week sounds like time, but sorry, I admited I don't fully understand GetBlockProofEquivalentTime so I shouldn't have made the time vs height comment and I bet you're right it was irrelevant
103 2017-01-13 04:06:22	0|BlueMatt|uhhh, wut....just fetched from github and compared my local branch between my two machines as always....except this time they're different
104 2017-01-13 04:06:39	0|MarcoFalke|huh?
105 2017-01-13 04:06:48	0|MarcoFalke|same commit hashes?
106 2017-01-13 04:06:54	0|BlueMatt|different commit hash, same tree
107 2017-01-13 04:07:34	0|BlueMatt|oh, nvm, false alarm
108 2017-01-13 04:22:48	0|cfields|BlueMatt: I suppose you just added cs_sendProcessing for general correctness? afaik, with the current single-threaded processing, there's no actual need for it?
109 2017-01-13 04:23:07	0|BlueMatt|https://github.com/bitcoin/bitcoin/pull/9535/commits/11290734ca7261f732c9f43f152c69de3a42546c
110 2017-01-13 04:23:11	0|BlueMatt|see commit body :p
111 2017-01-13 04:23:17	0|BlueMatt|is only ever taken on the one MessageHandler thread, but because
112 2017-01-13 04:23:17	0|BlueMatt|"Technically cs_sendProcessing is entirely useless now because it
113 2017-01-13 04:23:17	0|BlueMatt|there may be multiple of those in the future, it is left in place"
114 2017-01-13 04:23:23	0|BlueMatt|but, yea, its for correctness
115 2017-01-13 04:23:25	0|cfields|haha
116 2017-01-13 04:23:32	0|cfields|at least we agree :)
117 2017-01-13 04:23:40	0|BlueMatt|true
118 2017-01-13 04:23:42	0|cfields|serves me right for not reading. thanks.
119 2017-01-13 04:25:53	0|cfields|it'd be nice if github emphasized the display of full individual commit messages better. I'm well aware that I put much more effort into writing them than reviewers put into reading them. And I'm equally guilty in my own reviews.
120 2017-01-13 04:26:31	0|BlueMatt|too easy to see what the author intended instead of what it actually does
121 2017-01-13 04:26:54	0|BlueMatt|I tend to read the commit message if I'm confused to see if it helps me understand, but only if I have an idea that it might be broken
122 2017-01-13 04:27:33	0|cfields|fair point
123 2017-01-13 04:27:45	0|BlueMatt|ok, I'm down to 4 prs to review tomorrow...should be fun
124 2017-01-13 04:30:20	0|cfields|heh
125 2017-01-13 04:30:23	0|cfields|feeling better, i hope?
126 2017-01-13 04:30:40	0|BlueMatt|yea, well enough to review, at least
127 2017-01-13 04:30:47	0|BlueMatt|taking a day off to lie in bed helped a ton
128 2017-01-13 04:35:04	0|cfields|good
129 2017-01-13 04:35:15	0|cfields|those days are helpful even when you're not sick :)
130 2017-01-13 04:36:25	0|cfields|maybe not in bed all day, but disconnected enough to make you crave code again
131 2017-01-13 04:36:42	0|BlueMatt|oh, didnt mean to imply I didnt have email to respond to all day :p
132 2017-01-13 04:37:46	0|cfields|oh, heh
133 2017-01-13 07:35:27	0|jonasschnelli|sipa: is this (https://github.com/sipa/bitcoin-seeder/pull/42) related to the seeder silent crash we recently encountered?
134 2017-01-13 07:57:07	0|fanquake|Has anyone running master noticed sync/reindex stalling just prior to getting a connection refused message in the debug.log?
135 2017-01-13 07:58:34	0|fanquake|Have just had one instance of the GUI freezing/lagging, and no debug output for ~10 seconds. Then a few UpdateTip messages followed by a Connection refused.
136 2017-01-13 07:59:12	0|fanquake|Testing 9484 btw, I think I'll get a full sync in just less than 2 hours.
137 2017-01-13 07:59:29	0|fanquake|1hr40m to block 424'000
138 2017-01-13 08:00:20	0|gmaxwell|fanquake: my first guess is the connection refused is just that the sleep that drove it hit its timeout while the slow operation happened, so it happened immediately after update tip.
139 2017-01-13 08:01:02	0|gmaxwell|fanquake: and I would assume the updatetip took a while whole holding CS main because it was processing a bunch of blocks at once due to a reodering.
140 2017-01-13 08:01:50	0|gmaxwell|e.g. you get a bunch of blocks with one massively out of order, when you finally fill in that missing block it goes and connects 100 at a time holding locks the whole time, which stalls the gui and causes some loss of network transfer speed.
141 2017-01-13 08:08:02	0|gmaxwell|fanquake: there may be some more interesting bug lurking for sure... but if so I wouldn't notice it because I'd discount it because we already know that ProcessNewBlock can have very high latency.
142 2017-01-13 08:53:01	0|fanquake|gmaxwell thanks for the explanation. I'll collect a few more details and open an issue to track it.
143 2017-01-13 08:53:26	0|fanquake|gmaxwell btw, final sync time with 9484 -dbcache=4096 -par=8 was 2h12m
144 2017-01-13 08:53:47	0|fanquake|s/sync/-reindex-chainstate
145 2017-01-13 08:53:47	0|gmaxwell|you might want to add some logs around proceessnew block, e.g. enter and exit times. to try to validate my theory.
146 2017-01-13 08:54:00	0|gmaxwell|fanquake: fantastic.
147 2017-01-13 08:54:19	0|fanquake|That was to block 447885.
148 2017-01-13 08:56:22	0|gmaxwell|we could also consider adding a locking debug mode that saves the time of lock/unlock and prints if a lock is held more than (say) 1 second by a single piece of code.
149 2017-01-13 11:30:08	0|cjamthagen|Are timelocked transactions included in your mempool if they are not yet valid?
150 2017-01-13 13:32:59	0|morcos|cjamthagen: no, they are not
151 2017-01-13 14:06:43	0|morcos|luke-jr: just want to be sure you realize in #9519 it's not all txs that signal opt-in RBF that are excluded, but only actual replacements, regardless of whether they themselves signal opt-in RBF.
152 2017-01-13 14:06:45	0|gribble|https://github.com/bitcoin/bitcoin/issues/9519 | Exclude RBF replacement txs from fee estimation by morcos · Pull Request #9519 · bitcoin/bitcoin · GitHub
153 2017-01-13 14:49:37	0|bitcoin-git|[13bitcoin] 15jnewbery opened pull request #9542: Docs: Update CONTRIBUTING.md (06master...06CONTRIBUTINGcomponents) 02https://github.com/bitcoin/bitcoin/pull/9542
154 2017-01-13 14:50:38	0|jeremyru1in|This is not really critical, but as jtimon points out there is a lot of confusion over "Trivial" tags for PR's. https://github.com/bitcoin/bitcoin/blob/03dd707dc027fbf6f24120213f8eb66571600374/CONTRIBUTING.md is the closest thing to a specification of what Trivial means. I think that we'd save ourselves a lot of confusion if we adopted a policy that was more in line with how people typically (and in many other projects) use "trivial
155 2017-01-13 14:50:45	0|jeremyru1in|oops
156 2017-01-13 14:50:54	0|jeremyru1in|I see jnewbery has just opened something on that, will move my message there
157 2017-01-13 15:07:26	0|bitcoin-git|[13bitcoin] 15laudaa opened pull request #9543: [Trivial] Grammar and typo correction (06master...06master) 02https://github.com/bitcoin/bitcoin/pull/9543
158 2017-01-13 15:16:20	0|sipa|jeremyru1in: i'm sure we had that written down somewhere, but i can't find it anymore
159 2017-01-13 15:16:34	0|sipa|i also can't find the description of utACK etc anymore, which used to be in some document
160 2017-01-13 15:17:14	0|achow101|sipa: it's still there: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#peer-review
161 2017-01-13 15:17:24	0|sipa|ah!
162 2017-01-13 15:21:56	0|achow101|is the sync overlay thing supposed to show up on windows? because I don't see it
163 2017-01-13 15:27:36	0|gmaxwell|cjamthagen: only if they'll be valid at the next block or sooner.
164 2017-01-13 15:28:11	0|sipa|jonasschnelli: yes, that patch fixes the lack of dns response
165 2017-01-13 15:29:23	0|jonasschnelli|sipa: Great.
166 2017-01-13 15:44:57	0|instagibbs|can I PR directly against a BIP or should I bug the author?
167 2017-01-13 15:49:32	0|jtimon|instagibbs: not sure what's the norm, but you definitely can technically, I received some PRs to my PR for bip99
168 2017-01-13 15:49:46	0|jonasschnelli|instagibbs: I think you can PR but don't expect a merge without authors ack
169 2017-01-13 15:50:05	0|jtimon|oh, you mean PR directly to the repo, not his own PR
170 2017-01-13 15:50:32	0|jtimon|yeah then what jonasschnelli said
171 2017-01-13 16:00:29	0|sdaftuar|BlueMatt: not sure you saw my comment in #9375 -- i suggested adding a "to do" comment in ProcessGetData, to remind us of the issues we discussed in requests for invalid blocks.  thoughts?
172 2017-01-13 16:00:32	0|gribble|https://github.com/bitcoin/bitcoin/issues/9375 | Relay compact block messages prior to full block connection by TheBlueMatt · Pull Request #9375 · bitcoin/bitcoin · GitHub
173 2017-01-13 16:10:44	0|paveljanik|Unicorn...
174 2017-01-13 16:11:57	0|luke-jr|morcos: oh, I didn't realise that. But we can't know if they are or aren't replacements necessarily?
175 2017-01-13 16:12:44	0|instagibbs|RIP github
176 2017-01-13 16:12:52	0|BlueMatt|annnddd github down
177 2017-01-13 16:12:54	0|instagibbs|anyone need coffee
178 2017-01-13 16:12:59	0|BlueMatt|yes!
179 2017-01-13 16:14:31	0|paveljanik|good idea!
180 2017-01-13 16:14:37	0|BlueMatt|sdaftuar: hum, I did miss that comment
181 2017-01-13 16:15:11	0|BlueMatt|sdaftuar: remind me of the context again?
182 2017-01-13 16:18:20	0|BlueMatt|for those bored during the github outage, https://0bin.net/paste/gPzFQpbXtDj9hQO8#NG+mcjz5Xuro4XPK4ZijCtV0sE4U-nFdoi8AWBL1IZX is #9535 and while its not tagged 0.14, its itself a big win and it would be swell if it could go in
183 2017-01-13 16:18:21	0|gribble|https://github.com/bitcoin/bitcoin/issues/9535 | HTTP Error 503: Service Unavailable
184 2017-01-13 16:18:30	0|BlueMatt|:p
185 2017-01-13 16:20:00	0|BlueMatt|(is also super easy to review)
186 2017-01-13 16:25:04	0|sdaftuar|BlueMatt: general issue is that we don't have a way to tell a peer that we're intentionally ignoring a request, so our peer can't distinguish stalling from "sorry i now think this block is invalid"
187 2017-01-13 16:25:27	0|sdaftuar|i think we talked about eventually extending the p2p protocol to communicate this, potentially?
188 2017-01-13 16:25:52	0|sdaftuar|but documenting that we might announce a block and then ignore the request in ProcessGetData() seems prudent
189 2017-01-13 16:26:08	0|BlueMatt|I'm not sure we talked about it, but, yea, thats something to consider...
190 2017-01-13 16:26:25	0|luke-jr|if github is down a day, do we defer the freeze a day too? :P
191 2017-01-13 16:26:27	0|BlueMatt|we do, technically, have reject messages, but ignore them because they're already not serialized with the rest of the p2p protocol
192 2017-01-13 16:26:47	0|BlueMatt|luke-jr: I vote yes (and will do review today either way :p)
193 2017-01-13 16:26:54	0|sdaftuar|right, we could extend the reject message generation to also apply to getdata messages
194 2017-01-13 16:27:01	0|sdaftuar|rather than just block/tx messages
195 2017-01-13 16:27:09	0|luke-jr|I wish GitHub's review thing worked offline
196 2017-01-13 16:29:32	0|achow101|it looks like they're back
197 2017-01-13 16:30:52	0|BlueMatt|sdaftuar: we'd probably have to also fix the reject messages so that they are serialized
198 2017-01-13 16:30:59	0|BlueMatt|instead of at some random time in the future, who knows
199 2017-01-13 16:33:34	0|morcos|luke-jr: yes we do know that... look at the code when GitHub is back
200 2017-01-13 16:34:53	0|sipa|going over 9441 a last time
201 2017-01-13 16:45:19	0|BlueMatt|sdaftuar: actually, if we're gonna extend it, we should just throw out the reject messages we have now entirely (no one uses them, they have some simple design flaws, and they are a big anonymity issue)
202 2017-01-13 16:45:29	0|BlueMatt|sdaftuar: then we can add something that says "I will not respond to your block request"
203 2017-01-13 16:45:46	0|BlueMatt|but we will send such a message only if we already announced said block
204 2017-01-13 16:45:51	0|BlueMatt|(and this would not apply to transactions)
205 2017-01-13 17:03:00	0|BlueMatt|someone wanna kick travis for #9484? sipa or wumpus, though I think gmaxwell can do it too
206 2017-01-13 17:03:02	0|gribble|https://github.com/bitcoin/bitcoin/issues/9484 | Introduce assumevalid setting to skip validation presumed valid scripts. by gmaxwell · Pull Request #9484 · bitcoin/bitcoin · GitHub
207 2017-01-13 17:16:15	0|sipa|i'll do so in a bit
208 2017-01-13 17:50:51	0|bitcoin-git|[13bitcoin] 15JeremyRubin closed pull request #9503: listreceivedbyaddress Filter Address (06master...06listreceivedbyaddress-filtered) 02https://github.com/bitcoin/bitcoin/pull/9503
209 2017-01-13 17:52:08	0|jeremyru1in|Ugh that's annoying that it reports that here ^^; just did that to force travis to restart Z(failed during the github downtime due to not being able to hit github)
210 2017-01-13 17:59:45	0|luke-jr|jonasschnelli: would it help if I go over my suggestions for 9294 and do them myself?
211 2017-01-13 18:01:20	0|sipa|BlueMatt: restart
212 2017-01-13 18:02:38	0|gmaxwell|jeremyru1in: force push also does that.
213 2017-01-13 18:02:43	0|bitcoin-git|[13bitcoin] 15sipa pushed 17 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/02e5308c1b9f...8b66bf74e2a3
214 2017-01-13 18:02:44	0|bitcoin-git|13bitcoin/06master 1453ad9a1 15Cory Fields: net: fix typo causing the wrong receive buffer size...
215 2017-01-13 18:02:44	0|bitcoin-git|13bitcoin/06master 14e5bcd9c 15Cory Fields: net: make vRecvMsg a list so that we can use splice()
216 2017-01-13 18:02:45	0|bitcoin-git|13bitcoin/06master 145b4a8ac 15Cory Fields: net: make GetReceiveFloodSize public...
217 2017-01-13 18:02:50	0|BlueMatt|wooooo
218 2017-01-13 18:02:57	0|bitcoin-git|[13bitcoin] 15sipa closed pull request #9441: Net: Massive speedup. Net locks overhaul (06master...06connman-locks) 02https://github.com/bitcoin/bitcoin/pull/9441
219 2017-01-13 18:03:02	0|BlueMatt|c-c-c-combo-breaker
220 2017-01-13 18:03:21	0|jeremyru1in|nooo matt now nothing will get merged
221 2017-01-13 18:04:58	0|BlueMatt|sipa: I'm too lazy to squash #9375 (and each commit passes the "it works, and passes test" rule), fyi, in case you were planning on waiting for that
222 2017-01-13 18:05:01	0|gribble|https://github.com/bitcoin/bitcoin/issues/9375 | Relay compact block messages prior to full block connection by TheBlueMatt · Pull Request #9375 · bitcoin/bitcoin · GitHub
223 2017-01-13 18:13:15	0|jeremyrubin|gmaxwell: yeah, I was just lazy and on github.com at the moment :/
224 2017-01-13 18:28:22	0|cfields|\o/
225 2017-01-13 18:29:42	0|cfields|BlueMatt: 9535 needs rebase
226 2017-01-13 18:35:16	0|BlueMatt|cfields: done
227 2017-01-13 18:35:29	0|cfields|thanks
228 2017-01-13 19:16:09	0|sipa|ugh, master broken in p2p-segwit?
229 2017-01-13 19:16:27	0|sipa|i tested locally before merge
230 2017-01-13 19:24:03	0|bitcoin-git|[13bitcoin] 15practicalswift opened pull request #9544: Add end of namespace comments. Improve consistency for anonymous namespaces. (06master...06consistent-use-of-end-of-namespace-comments) 02https://github.com/bitcoin/bitcoin/pull/9544
231 2017-01-13 19:28:27	0|bitcoin-git|[13bitcoin] 15practicalswift opened pull request #9545: Add override:s where appropriate (06master...06add-overrides-where-appropriate) 02https://github.com/bitcoin/bitcoin/pull/9545
232 2017-01-13 19:30:28	0|cfields|sipa: pebkac, i hope?
233 2017-01-13 19:31:58	0|sipa|cfields: ?
234 2017-01-13 19:32:33	0|cfields|<sipa> ugh, master broken in p2p-segwit?
235 2017-01-13 19:35:43	0|sipa|i assume it's somehow a spurious travis error
236 2017-01-13 19:35:50	0|sipa|not pebcak
237 2017-01-13 19:36:53	0|cfields|oh, i didn't see the failure, thought you were seeing it locally. got a link?
238 2017-01-13 19:42:03	0|sipa|you don't get an email?
239 2017-01-13 19:42:31	0|sipa|https://travis-ci.org/bitcoin/bitcoin/builds/191711996
240 2017-01-13 19:42:45	0|sipa|though i think the log may be unavailable as i've restarted
241 2017-01-13 20:00:36	0|BlueMatt|grrr, I hate reviewing wallet changes
242 2017-01-13 20:03:09	0|gmaxwell|BlueMatt: why? it use to be more obnoxious but I think it's no big deal now.
243 2017-01-13 20:03:46	0|BlueMatt|because unlike some of our other codepaths, any change to eg spendability (like bumpfee) means reviewing like 20 functions which use spendability in slightly different ways
244 2017-01-13 20:11:05	0|morcos|BlueMatt: I think its useful to look at it similar to how sdaftuar suggested.  imagine you stop your node, you restart it, and before your wallet can try to reaccept your old transaction an external replacement comes in.  its not required to _do anything_ to affect whether your orig tx affects spendability.
245 2017-01-13 20:12:49	0|morcos|All bumpfee is doing is adding an extra layer of conservativeness on top...  You know what... if you replace a tx, you're never going to be allowed to chain unconfirmed txs on the original tx again..
246 2017-01-13 20:18:09	0|BlueMatt|morcos, sdaftuar: I'm not convinced that not allowing a user to spend an input they removed from a replacement is The Right Behavior, especially as we move towards more loose replacement policies, but I'm fine with that for now. I don't see how thats massively different from an abandon (which means "pretend this isnt here, unless it somehow gets confirmed")
247 2017-01-13 20:20:04	0|gmaxwell|BlueMatt: bumpfee doesn't change the inputs. And any released inputs should not be spendable until the other spend is clearly conflicted.
248 2017-01-13 20:20:06	0|morcos|I think we're conflating two different things here...  Whether you can chain transactions off the original tx (the only change made in bumpfee) and whether any inputs spent by the original tx are still considered spent by the original tx (this is treated the same way any double spend is, both spends are spends)
249 2017-01-13 20:20:22	0|BlueMatt|gmaxwell: yes, this was essentially all theoretical as it cant happen yet
250 2017-01-13 20:20:32	0|gmaxwell|since bumpfee can't do that right now we can ignore having to handle the case where it does become spendable yet..
251 2017-01-13 20:20:56	0|morcos|If you want inputs spent by the original tx to not count as spent, there is nothing stopping you from manually abandoning the original tx, but i can't possibly imagine thinking it was a good idea to happen in an automatic fashion
252 2017-01-13 20:22:13	0|gmaxwell|morcos: well it should abandon the replacement or replaced txn automatically once it is well conflicted, I think? (doesn't need to now, but eventual functionality)
253 2017-01-13 20:23:03	0|BlueMatt|morcos: I think eventually if we have some super fancy gui that allows you to replace a transaction, and you swap the inputs selected, it woud be weird to not be able to re-spend them, as you can for abandon
254 2017-01-13 20:23:07	0|BlueMatt|but, agreed, lets table for now
255 2017-01-13 20:23:14	0|BlueMatt|the thing it does now is the more conservative option
256 2017-01-13 20:23:23	0|BlueMatt|which is good, and its a moot point unless we have other features added
257 2017-01-13 20:23:41	0|gmaxwell|BlueMatt: are you saying you should be immediately able to respend them in that case?
258 2017-01-13 20:24:04	0|BlueMatt|gmaxwell: dunno, probably not? unclear to me...but, lets table for now
259 2017-01-13 20:24:26	0|gmaxwell|BlueMatt: if so that would be a huge footgun, I think, as it would assume the user has a better mental model for what gets confirmed than we know they do. (than even many of us do much of the time).
260 2017-01-13 20:24:29	0|BlueMatt|but should be able to respend them after the replacement confirms, which you cannot do now
261 2017-01-13 20:24:44	0|BlueMatt|gmaxwell: yea, well abandon is the same thing :p
262 2017-01-13 20:24:52	0|morcos|gmaxwell: but that happens automatically right?
263 2017-01-13 20:25:12	0|BlueMatt|morcos: my reading of the code now, you cannot ever respend the inputs which are no longer being replaced
264 2017-01-13 20:25:26	0|morcos|if A spends inputs 1 , 2, 3 and you use futurebumpfee to replace A with A' which spends inputs 2, 3, 4
265 2017-01-13 20:25:27	0|sdaftuar|yeah when the replacement confirms, the original will be cobnflicted, freeing up the inputs
266 2017-01-13 20:25:58	0|morcos|then until either are confirmed, 1,2,3, and 4 will appear spent.  When A' confirms, input 1 will automatically become spendable again because A is conflicted.
267 2017-01-13 20:26:25	0|morcos|this seems like roughly the right behavior, and more importantly not a change from the existing behavior
268 2017-01-13 20:27:08	0|morcos|but maybe BlueMatt is saying there is a bug and thats not happening?
269 2017-01-13 20:27:10	0|BlueMatt|sdaftuar: oh, youre right, sorry missed that
270 2017-01-13 20:27:17	0|BlueMatt|morcos: nope, misreading, I think
271 2017-01-13 20:28:13	0|BlueMatt|sdaftuar: I do think we need the RelayWalletTransaction isReplaced gate: https://github.com/bitcoin/bitcoin/pull/8456#discussion_r96068068
272 2017-01-13 20:28:20	0|BlueMatt|if we ever change the min relay fee it could blow up
273 2017-01-13 20:28:44	0|gmaxwell|need morcos feesplit
274 2017-01-13 20:29:16	0|BlueMatt|gmaxwell: was that a please-review reminder?
275 2017-01-13 20:29:25	0|BlueMatt|(for #9380)
276 2017-01-13 20:29:27	0|gribble|https://github.com/bitcoin/bitcoin/issues/9380 | Separate different uses of minimum fees by morcos · Pull Request #9380 · bitcoin/bitcoin · GitHub
277 2017-01-13 20:29:32	0|BlueMatt|:p
278 2017-01-13 20:29:36	0|morcos|BlueMatt: I'm not sure I agree with that either...
279 2017-01-13 20:29:39	0|morcos|In the case A
280 2017-01-13 20:29:42	0|morcos|sorry
281 2017-01-13 20:30:12	0|morcos|In the case A' becomes conflicted... you want to Relay A, so I think that means you always want to relay A
282 2017-01-13 20:30:41	0|morcos|it would be nice if you first try to reaccept everything and then only relay them if they are in your mempool at the end
283 2017-01-13 20:30:58	0|gmaxwell|we shouldn't be relaying multiple verions of conflicting transactions at a time. but the mempool will make sure we don't.
284 2017-01-13 20:31:16	0|morcos|I _think_ that'll be mostly what happens in practice, but i guess its not guaranteed.. (so assuming you have both A and A' probably only the second will be relayeD)
285 2017-01-13 20:32:06	0|BlueMatt|lets say you have A and its been replaced by A' -> normally you restart, A (might be) accepted first, then A' replaces it, like normal
286 2017-01-13 20:32:22	0|BlueMatt|I think this does mean we'll announce A, but proobably not if its been delayed-announced
287 2017-01-13 20:32:38	0|BlueMatt|however, if the user restarts with a higher min relay fee, A' might not meet the replacement requirements
288 2017-01-13 20:32:43	0|BlueMatt|and so A ends up in your mempool
289 2017-01-13 20:32:44	0|BlueMatt|which sucks
290 2017-01-13 20:32:44	0|morcos|isn't it always delayed-announced
291 2017-01-13 20:33:01	0|BlueMatt|dont think so?
292 2017-01-13 20:33:07	0|sdaftuar|BlueMatt: i agree that is a possible edge case
293 2017-01-13 20:33:12	0|morcos|i mean you're almost right.. A' by definition has a higher relay fee
294 2017-01-13 20:33:27	0|morcos|but you could restart with a higher dust limit, which could cause A to be accepted and not A'
295 2017-01-13 20:33:35	0|BlueMatt|morcos: huh? relay fee is a property of your software, not the transacion?
296 2017-01-13 20:33:37	0|morcos|but in that case you probably want A to be relayed!
297 2017-01-13 20:33:46	0|BlueMatt|yes, dust limit is calculated from minrelayfee, iirc
298 2017-01-13 20:33:48	0|gmaxwell|we probably should sort the unconfirmed transaction by dependency/relay fee before inserting, instead of just by order. but I don't think thats urgent now.
299 2017-01-13 20:33:50	0|morcos|A' by definition has a higher feerate
300 2017-01-13 20:34:08	0|morcos|gmaxwell: agreed.. not urgent...  this is like rare edge case
301 2017-01-13 20:34:27	0|morcos|and even if both get relayed or the wrong one, its not necessarily a BAD thing
302 2017-01-13 20:34:33	0|BlueMatt|I think its an easy solution to just add a replaced_by check prior to relay?
303 2017-01-13 20:34:41	0|morcos|but that has downsides
304 2017-01-13 20:34:47	0|BlueMatt|ehh...I mean I think once you've replaced you probably want the replacement relayed
305 2017-01-13 20:34:47	0|sdaftuar|that opens the door to morcos' edge case
306 2017-01-13 20:35:04	0|BlueMatt|like, if you restart and it has a lower feerate that gets stuck in your mempool, that sucks and just means you're gonna have to replace again
307 2017-01-13 20:35:24	0|BlueMatt|plus you might've done something else in the replace, like set it to not-replaceable
308 2017-01-13 20:35:28	0|BlueMatt|or whatever
309 2017-01-13 20:35:30	0|morcos|but the only reason the old one is stuck is there is something wrong wiht the new one
310 2017-01-13 20:35:44	0|morcos|in that case you are basically just screwed anyway, unless you replace the new one
311 2017-01-13 20:36:16	0|BlueMatt|not really...? the new one is just as fine as the old one
312 2017-01-13 20:36:30	0|morcos|then why is that not replacing the old one in your mempool?
313 2017-01-13 20:36:31	0|BlueMatt|just maybe that the new one wont auto-replace the old one in peoples' mempool across the network
314 2017-01-13 20:36:38	0|morcos|don't you have chicken soup to eat?
315 2017-01-13 20:36:47	0|BlueMatt|heh
316 2017-01-13 20:36:55	0|morcos|oh b/c of the incremental relay fee getting raised
317 2017-01-13 20:37:03	0|morcos|i missed that that was your point
318 2017-01-13 20:37:06	0|BlueMatt|because either a) you're manually setting a higher -minrelayfee (for some reason...)...or b) you updated to a new version with a higher -minrelayfee default
319 2017-01-13 20:37:15	0|BlueMatt|oh, sorrry, yes, that was my point
320 2017-01-13 20:37:34	0|BlueMatt|so maybe the replacement wont relay that well depending on how much the rest of the network has upgraded
321 2017-01-13 20:37:35	0|morcos|sure.. but look.. you shouldn't NEED to rerelay either of them
322 2017-01-13 20:37:43	0|morcos|just b/c you restarted your node, doesn't mean everyone else did
323 2017-01-13 20:37:45	0|BlueMatt|huh???
324 2017-01-13 20:38:00	0|BlueMatt|and if you're (bumped) fee is low enough that it'll take a few days to get in?
325 2017-01-13 20:38:07	0|BlueMatt|we cant break re-announce???
326 2017-01-13 20:38:31	0|BlueMatt|eg a key use-case for bumpfee is setting the fee super low and bumping it eventually if it doesnt get in for a few days
327 2017-01-13 20:38:32	0|morcos|we're not breaking it.. we're just saying there is a contrived edge case where it might not be super effective
328 2017-01-13 20:38:33	0|BlueMatt|or a week
329 2017-01-13 20:38:45	0|BlueMatt|whats the downside of adding the check to relay???
330 2017-01-13 20:38:52	0|BlueMatt|it fixes an edge case, and....?
331 2017-01-13 20:39:28	0|BlueMatt|eg I use bumpfee in greenaddress to send transactions with super low feerate to transfer between my wallets cause I dont care about conf time
332 2017-01-13 20:39:34	0|sdaftuar|if you have upgraded settings so that the new tx is not sufficiently above the old one's feerate to relay through your own node, you might as well assume that you have to fee bump again to get it to relay across the network
333 2017-01-13 20:39:35	0|BlueMatt|and if it never gets in, i just bump it slightly
334 2017-01-13 20:39:35	0|morcos|i think the right way to do it is what gmaxwell said...  some sort of sort..  where you follow the chain of A->A'->A'' to the end and then start by trying to rebroadcast A''
335 2017-01-13 20:39:55	0|BlueMatt|sdaftuar: nope, it could time out, or maybe you're on a new version that much of the network isnt
336 2017-01-13 20:39:59	0|morcos|but thats an improvement that can be done later
337 2017-01-13 20:40:12	0|BlueMatt|(or you manually set -minrelayfee higher, which a bunch of folks did when we didnt have limited mempool and likely still have)
338 2017-01-13 20:40:16	0|morcos|just blindly saying skip A if we have A' is not obviously better to me
339 2017-01-13 20:40:19	0|sdaftuar|what would you have done if you upgraded before the bumpfee call?
340 2017-01-13 20:40:52	0|BlueMatt|morcos: so you announce all three txn potentially to your peers? that blows
341 2017-01-13 20:41:16	0|sdaftuar|BlueMatt: that is already what your recipient(s) will do
342 2017-01-13 20:41:18	0|morcos|how so... relay happens to run in the middle of reacceptWalletTransactions
343 2017-01-13 20:41:19	0|BlueMatt|yea, also what about announce? I'm pretty sure there are a few not-delayed-announce cases?
344 2017-01-13 20:41:45	0|morcos|ok, how about this for a fix
345 2017-01-13 20:41:57	0|morcos|its hacky but i think strictly better
346 2017-01-13 20:42:00	0|BlueMatt|(also because we're likely to change relay in the future to do some fast-path'ing stuff through some nodes)
347 2017-01-13 20:42:16	0|morcos|on startup only, we call ReaccpetWalletTransactions with a bool which skips txs that have been replaced
348 2017-01-13 20:42:39	0|gmaxwell|BlueMatt: maybe the replacement is non-standard?
349 2017-01-13 20:42:42	0|morcos|bc the replacement might be bad/conflicted/soemthing wrong with it
350 2017-01-13 20:43:00	0|gmaxwell|it could, btw with bumpfee... dust limit changing between restarts.
351 2017-01-13 20:43:02	0|BlueMatt|gmaxwell: fair...i suppose if the replacement fails to get accepted into our mempool we would want to try the original
352 2017-01-13 20:43:15	0|gmaxwell|In that case you get to keep both pieces, but ... it's worth discussing.
353 2017-01-13 20:43:51	0|morcos|yes this is the point...  but if we skip the replacees the first time through..  then they'll get skipped by virtue of the replacer already being in the mempool on future runs, and if for some reason it isn't.. then they'll get broadcast
354 2017-01-13 20:44:12	0|morcos|will lead to this order A'', A, A'   but that's an improvement
355 2017-01-13 20:44:13	0|BlueMatt|morcos: so you mean only do this on initial run? not on the timer-based one?
356 2017-01-13 20:44:16	0|morcos|yes
357 2017-01-13 20:44:24	0|BlueMatt|I'm happy with that
358 2017-01-13 20:44:32	0|morcos|I'm compromisey with that
359 2017-01-13 20:44:35	0|BlueMatt|it is super hacky
360 2017-01-13 20:44:44	0|morcos|that we can agree on
361 2017-01-13 20:44:49	0|sdaftuar|seems like a nice optimization for a futre pr
362 2017-01-13 20:44:52	0|BlueMatt|but should at least fix the issue while not (really) risking anything
363 2017-01-13 20:45:04	0|BlueMatt|sdaftuar: yea, which will be so chock-full of edge cases......
364 2017-01-13 20:45:07	0|BlueMatt|but, ok
365 2017-01-13 20:45:09	0|BlueMatt|sounds good
366 2017-01-13 20:45:17	0|morcos|yeah can we make that a bugfix PR to 8456
367 2017-01-13 20:45:24	0|morcos|so we can just get 8456 merged
368 2017-01-13 20:45:40	0|sdaftuar|haha
369 2017-01-13 20:46:30	0|BlueMatt|morcos: note how I did that to you when you asked me to change the logprints in #9375 :p
370 2017-01-13 20:46:33	0|gribble|https://github.com/bitcoin/bitcoin/issues/9375 | Relay compact block messages prior to full block connection by TheBlueMatt · Pull Request #9375 · bitcoin/bitcoin · GitHub
371 2017-01-13 20:47:05	0|morcos|good, i had no objection to that!
372 2017-01-13 20:51:05	0|morcos|BlueMatt: just looking at the code.. there is actually no way for relay to happen before all the txs have tried to be accepted to the mempool right?
373 2017-01-13 20:54:23	0|cfields|morcos: mind fixing up the mempool minReasonableRelayFee while you're messing around there? as i read the code, it doesn't respect -minrelaytxfee
374 2017-01-13 20:54:55	0|cfields|(i'm not familiar enough with all of the interactions to know if that matters much)
375 2017-01-13 20:55:05	0|BlueMatt|morcos: not sure? havent looked in a while?
376 2017-01-13 20:55:12	0|BlueMatt|doesnt really fully address the issue, though
377 2017-01-13 20:55:40	0|morcos|right still your edge case of A' no longer being able to replace A, but you think its better if you try to rebroadcast only A' in that case?
378 2017-01-13 20:56:49	0|morcos|it seems to me if that happens, then its likely b/c you raised the required increment (probably b/c a lot of other people did too) so even if you broadcast A' it might not propagate and you might be more likely to figure that out if you have A in your mempool instead of A'
379 2017-01-13 20:58:19	0|morcos|cfields: you are right it doesn't respect it...  but it actually is better that it doesn't i think...  it should probably just be cleaned up separtely in another fee estimation clean up.
380 2017-01-13 20:58:42	0|BlueMatt|morcos: hmmmm
381 2017-01-13 20:59:20	0|cfields|morcos: ok good, I was hoping you'd say that. So it should just take DEFAULT_MIN_RELAY_TX_FEE instead?
382 2017-01-13 20:59:21	0|morcos|i thought through what i thought it should be a couple of weeks ago, but now i can't exactly remember....
383 2017-01-13 20:59:50	0|morcos|almost.... i think if you ever want a minrelaytxfee less than DEFAULT, then you'd probably want that number to be less
384 2017-01-13 21:00:14	0|morcos|but maybe we can just worry abou tthat when it happens...  and probably best to just change it to DEFAULT_MIN_RELAY_TX_FEE for now
385 2017-01-13 21:01:07	0|morcos|or its own fee estimation constant... its basically used for determining the bucket sizes... so don't want to change it b/c then your old data file is useless
386 2017-01-13 21:01:10	0|bitcoin-git|[13bitcoin] 15practicalswift opened pull request #9547: Avoid potential division by zero in benchmark::State::KeepRunning() (06master...06avoid-potential-division-by-zero-in-benchmark-state-keeprunning) 02https://github.com/bitcoin/bitcoin/pull/9547
387 2017-01-13 21:01:30	0|morcos|ok maybe i should PR a change for it while we're thinking about it
388 2017-01-13 21:02:23	0|cfields|morcos: heh, yes please :). No problem with doing it later, i just see it now and then and make a mental note to ask you, and have never managed to do so.
389 2017-01-13 21:02:29	0|BlueMatt|morcos: I mean there's a strong argument for both - if you replaced a transaction, you really dont want to be relaying an old version out...on the flip side, if you replace a transaction and you wouldn't have accepted the replacement, maybe you want to know that...I think for my use-case (slowly bumping fee on a tx that I dont care when it confirms), I prefer the second - keep relaying the bumped one, especially because if the
390 2017-01-13 21:02:29	0|BlueMatt|original eventually times out on other folks' mempool's, I'd want the one with ever-so-sligly-higher fee to relay out and try to get confirmed
391 2017-01-13 21:02:34	0|BlueMatt|instead of the original one
392 2017-01-13 21:02:40	0|BlueMatt|because I obviously bumped it for a reason
393 2017-01-13 21:03:18	0|morcos|heh timeout is 2 weeks now..  you're a patient man
394 2017-01-13 21:04:50	0|BlueMatt|oh, i thought we set it to 1
395 2017-01-13 21:04:51	0|BlueMatt|oh well
396 2017-01-13 21:05:00	0|BlueMatt|there goes that argument
397 2017-01-13 21:07:06	0|BlueMatt|morcos: so technically the first add-all-wallet-txn-to-mempool is after CConnmanStart
398 2017-01-13 21:07:18	0|BlueMatt|(its in postInitProcess, the very last thing called in AppInit
399 2017-01-13 21:07:21	0|BlueMatt|)
400 2017-01-13 21:07:37	0|BlueMatt|so you could relay out old transactions if you get connections fast and have a massive wallet
401 2017-01-13 21:07:43	0|BlueMatt|theoretically, at least
402 2017-01-13 21:08:47	0|sdaftuar|if we did that after the load mempool from disk finishes, that might solve a lot of the problem?  m aybe messy to do that way, i haven't looked
403 2017-01-13 21:09:11	0|BlueMatt|hmm? dont think that would make it better?
404 2017-01-13 21:09:16	0|BlueMatt|oh, i see your point
405 2017-01-13 21:09:25	0|BlueMatt|i mean problem is that mempool-load is so super slow...
406 2017-01-13 21:09:28	0|sdaftuar|yeah
407 2017-01-13 21:09:53	0|sdaftuar|but there's kind of no hurry for this is there?  i was more concerned about layer violations
408 2017-01-13 21:11:01	0|morcos|it looks to me like the first resendwallettransactions doesn't happen for 30 mins
409 2017-01-13 21:11:02	0|BlueMatt|I mean I started by seeing the fact that your wallet might accept the pre-bump transaction into mempool after the bump as a bug....but y'all've convinced me that its at least conceivably the right outcome depending on what the user wants
410 2017-01-13 21:11:11	0|morcos|and thats the only way they get relayed
411 2017-01-13 21:11:20	0|morcos|just accepting them to the mempool doesn't relaythem
412 2017-01-13 21:13:31	0|BlueMatt|i think reaccepting post-mempool-disk-load still carries risk, though - some of the wallet logic depends on mempool-reading
413 2017-01-13 21:13:42	0|BlueMatt|so I wouldnt feel comfortable making that change without more than 2 days of review....
414 2017-01-13 21:19:48	0|gmaxwell|BlueMatt: we already try to reaccept wallet transactions continually.
415 2017-01-13 21:20:20	0|BlueMatt|yea?
416 2017-01-13 21:20:38	0|gmaxwell|yea. at the rebroadcast times.
417 2017-01-13 21:20:43	0|BlueMatt|I'm aware?
418 2017-01-13 21:21:07	0|BlueMatt|whats your point?
419 2017-01-13 21:21:27	0|gmaxwell|and the wallet 'works' without transactions in mempool... e.g. you can setup so that all transactions are rejected from the mempool.
420 2017-01-13 21:21:57	0|BlueMatt|my point was that we have wallet logic which depends on whether a transaction is in mempool, and if we're gonna change it so that you could spend a bunch more time at load with transactions not yet in mempool, that would require audit and careful thought about those places
421 2017-01-13 21:22:11	0|BlueMatt|I mean, yes, it works, but some things in it wont work
422 2017-01-13 21:22:14	0|BlueMatt|at least last I checked
423 2017-01-13 21:22:51	0|BlueMatt|even blocksonly puts wallet transactions in mempool, i think
424 2017-01-13 21:22:55	0|BlueMatt|just doenst relay them
425 2017-01-13 21:23:02	0|gmaxwell|BlueMatt: no it doesn't.
426 2017-01-13 21:23:10	0|gmaxwell|(unless you've enabled that specifically)
427 2017-01-13 21:23:49	0|BlueMatt|oh, heh, indeed it doesnt
428 2017-01-13 21:25:05	0|BlueMatt|yea, ok, looking at it again it looks like it would only disable features, not enable you to do anything you shouldnt be able to
429 2017-01-13 21:26:05	0|BlueMatt|anywayyyy
430 2017-01-13 21:53:27	0|morcos|BlueMatt: anywayyy.......... ->  ACK ?
431 2017-01-13 21:58:07	0|BlueMatt|morcos: getting there
432 2017-01-13 21:59:15	0|BlueMatt|morcos: ran out of steam so had to take a coffee break...digging into the meat now :p
433 2017-01-13 22:01:44	0|bitcoin-git|[13bitcoin] 15morcos opened pull request #9548: Remove min reasonable fee (06master...06removeMinReasonableFee) 02https://github.com/bitcoin/bitcoin/pull/9548
434 2017-01-13 22:22:40	0|luke-jr|would it be terrible, if wallets upon encounting a transaction they sent yet is still unconfirmed but is being spent already, were to double-spend their transaction to the latter destination(s)?
435 2017-01-13 22:23:06	0|luke-jr|eg, if I pay morcos, and see morcos paying BlueMatt before mine confirms, double-spend it to BlueMatt (and morcos's change address) directly
436 2017-01-13 22:23:36	0|BlueMatt|lol, lets not do that, I think
437 2017-01-13 22:23:46	0|BlueMatt|that has all kinds of fun potentials
438 2017-01-13 22:23:51	0|BlueMatt|"no, you didnt pay me, look at the blockchain!"
439 2017-01-13 22:24:05	0|luke-jr|:D
440 2017-01-13 22:24:16	0|luke-jr|save their transaction as proof
441 2017-01-13 22:24:29	0|BlueMatt|of course I'd appreciate that particular scenario, because I get all the btc :p
442 2017-01-13 22:24:37	0|BlueMatt|can we just hardcode my pubkey?
443 2017-01-13 22:24:39	0|luke-jr|haha
444 2017-01-13 22:29:51	0|luke-jr|would be a pain to implement in Core; maybe I'll make a highly experimental wallet that does crazy things like this if I ever get time
445 2017-01-13 22:30:39	0|sipa|ACK hardcoding BlueMatt's pubkey
446 2017-01-13 22:31:13	0|BlueMatt|morcos/ryanofsky: ok, so lets say you have tx A, then you bumpfee it to get A'....BUT someone has already built a tx on A (called B) (which you hadnt seen at the time of bump)....great, so now we have a scenario where, if A' times out of your mempool, it might get replaced with A (because you might see A+B from a peer prior to rebroadcasting A')
447 2017-01-13 22:31:38	0|luke-jr|only if I get a copy of his privkey
448 2017-01-13 22:31:40	0|sdaftuar|yes
449 2017-01-13 22:31:52	0|BlueMatt|now you have a few goofy things - like qt's getBalance is different from wallet's GetBalance (because AvailableCoins is different from pcoin->IsTrusted())
450 2017-01-13 22:32:06	0|BlueMatt|I know, I'm coming up with crazy edge cases now
451 2017-01-13 22:32:24	0|gmaxwell|luke-jr: autocuttrhough could be done, I suppose but you'd want to only do it with parties that opted into it.
452 2017-01-13 22:33:16	0|ryanofsky|don't understand the problem. if A gets added to a block then A' is conflicted and we don't care about it
453 2017-01-13 22:33:30	0|BlueMatt|no, not in a block
454 2017-01-13 22:33:35	0|BlueMatt|A' gets replaced in your mempool with A
455 2017-01-13 22:33:52	0|luke-jr|so?
456 2017-01-13 22:34:00	0|BlueMatt|(I came up with a convoluted case where that might happen, so now I'm gonna pretend we have to handle it gracefully :p)
457 2017-01-13 22:34:06	0|BlueMatt|luke-jr: well I dont think we handle that case gracefully
458 2017-01-13 22:34:59	0|sdaftuar|there's nothing you can really do right?
459 2017-01-13 22:35:11	0|BlueMatt|I think in this case qt's balance will suddenly forget about both txn
460 2017-01-13 22:35:23	0|BlueMatt|CWallet::GetBalance will just trust whatever's in your mempool, I think
461 2017-01-13 22:35:24	0|sdaftuar|if A has a high fee child spend, then A might well be more likely to be be mined than A'
462 2017-01-13 22:35:31	0|luke-jr|I don't see what the problem is?
463 2017-01-13 22:35:40	0|BlueMatt|sdaftuar: sure, but your balance shouldnt be different getween rpc and gui
464 2017-01-13 22:35:45	0|gmaxwell|sdaftuar: which change will it show in your balance?
465 2017-01-13 22:35:53	0|BlueMatt|let alone should the gui suddenly think the balance of both options will be 0
466 2017-01-13 22:35:54	0|sdaftuar|neither
467 2017-01-13 22:35:56	0|sdaftuar|i think
468 2017-01-13 22:35:57	0|sdaftuar|?
469 2017-01-13 22:36:05	0|sdaftuar|oh, i am not sure
470 2017-01-13 22:36:12	0|BlueMatt|but CWallet::GetBalance will show the one which is in your mempoo
471 2017-01-13 22:36:12	0|BlueMatt|l
472 2017-01-13 22:36:13	0|gmaxwell|E.g. A, A'  then A ends up back in your mempool.   Your balance doesn't go to zero when you spend coins and have change.
473 2017-01-13 22:36:29	0|gmaxwell|(if it did users would freak often)
474 2017-01-13 22:36:34	0|BlueMatt|rpc will list the balance assuming the one in your mempool gets confirmed, I /think/ gui would be 0
475 2017-01-13 22:36:49	0|BlueMatt|or, at least, WalletModel::getBalance would be 0
476 2017-01-13 22:36:53	0|BlueMatt|need to figure out what calls that
477 2017-01-13 22:37:23	0|gmaxwell|I think it's fine if balance reads a bit low, e.g. assumes you paid the largest of the fees. It's not okay if it goes to zero.
478 2017-01-13 22:37:48	0|BlueMatt|oh, no, its inconsistent, now I have no idea what this effects
479 2017-01-13 22:38:02	0|gmaxwell|Because it will cause someone to freak -- "I sent 1 bitcoin and by 10 BTC balance went to zero! where are all my coins! please help. I tried deleting my wallet 5 times and they haven't come back!"
480 2017-01-13 22:38:21	0|BlueMatt|ahh, ok, so nvm, what i think it does (because getBalance() is normally called without a coinControl) is that if you try to create a transaction it'll refuse to spend from either
481 2017-01-13 22:38:28	0|BlueMatt|and give you an "insufficient funds" error
482 2017-01-13 22:38:35	0|BlueMatt|but the display will be right
483 2017-01-13 22:38:41	0|gmaxwell|yes, thats fine. it's like spendzeroconfchange=0 for those inputs.
484 2017-01-13 22:38:42	0|BlueMatt|which is strange, but probably fine given its a crazy edge case
485 2017-01-13 22:39:31	0|BlueMatt|gmaxwell: you asked why I hated reviewing wallet prs? :p
486 2017-01-13 22:39:45	0|BlueMatt|billion and one possible corner cases
487 2017-01-13 22:39:53	0|gmaxwell|if you think this is easier that reviewing network or consensus changes, I am frightened. :P
488 2017-01-13 22:40:17	0|BlueMatt|harder than network? yea it is
489 2017-01-13 22:40:19	0|BlueMatt|consensus, ok, maybe not
490 2017-01-13 22:40:41	0|gmaxwell|there are just as many corner cases! we just mishandle them more often. :P
491 2017-01-13 22:40:55	0|BlueMatt|heh, ok, fair point
492 2017-01-13 22:43:10	0|BlueMatt|note: I still havent even fucking looked at the rpc code for bumpfee :'(
493 2017-01-13 22:43:43	0|BlueMatt|do we document that listunspent will not list the outputs from bumped transactions (they will dissapear after you bump and neither the bumped or unbumped version's outputs will show up)
494 2017-01-13 22:43:57	0|BlueMatt|which is super strange because it normally lists everything except abandoned outputs
495 2017-01-13 22:47:09	0|gmaxwell|outputs disappear anytime they're used in a spend, ... bumpfee is no behavior change there.
496 2017-01-13 22:47:30	0|BlueMatt|not in listunspent, no, I dont think so?
497 2017-01-13 22:47:47	0|gmaxwell|any output thats spent (including in an unconfirmed txn) is not included in list_un_spent. :)
498 2017-01-13 22:47:51	0|gmaxwell|oh do you mean the change?
499 2017-01-13 22:48:04	0|BlueMatt|yes, change, sorry
500 2017-01-13 22:48:26	0|gmaxwell|the change should probably still be there but marked held. :-/ basically anything in the balance computation sould be there.
501 2017-01-13 22:49:00	0|BlueMatt|I believe its also not listed in the gui's coincontrol options for inputs
502 2017-01-13 22:49:13	0|BlueMatt|but I'm guessing there because I'd need to go read a whole 'nother pile of code to double-check
503 2017-01-13 22:51:26	0|BlueMatt|yes, but which one should show up in that list?
504 2017-01-13 22:51:27	0|BlueMatt|:p
505 2017-01-13 22:51:33	0|BlueMatt|replaced or original?
506 2017-01-13 22:52:53	0|bitcoin-git|[13bitcoin] 15sipa pushed 15 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/8b66bf74e2a3...3908fc472805
507 2017-01-13 22:52:54	0|bitcoin-git|13bitcoin/06master 148017547 15Matt Corallo: Make CBlockIndex*es in net_processing const
508 2017-01-13 22:52:54	0|bitcoin-git|13bitcoin/06master 148baaba6 15Matt Corallo: [qa] Avoid race in preciousblock test....
509 2017-01-13 22:52:54	0|bitcoin-git|13bitcoin/06master 149a0b2f4 15Matt Corallo: [qa] Make compact blocks test construction using fetch methods
510 2017-01-13 22:52:57	0|BlueMatt|woooo
511 2017-01-13 22:53:06	0|bitcoin-git|[13bitcoin] 15sipa closed pull request #9375: Relay compact block messages prior to full block connection (06master...062016-12-compact-fast-relay) 02https://github.com/bitcoin/bitcoin/pull/9375
512 2017-01-13 22:53:07	0|BlueMatt|one more down for 0.14
513 2017-01-13 22:58:18	0|owowo|how about skipping 0.14 and got directly to 0.15? I mean _4_ sounds like death in Chinese. ;0)
514 2017-01-13 22:58:55	0|morcos|BlueMatt: ok i admit, that's some good finds.  i did test some of this stuff both in RPC and GUI, but I dont think I fully thought through all the ways different balances might differ
515 2017-01-13 22:59:20	0|morcos|Indeed change from bumper txs do not appear in coin control (as they shouldn't i think b/c you can't spend them)
516 2017-01-13 22:59:42	0|sipa|cfields: i think we should detect whether the compiler supports -fsanitize (gcc 4.8 added -fsanitize=address, 4.9 added -fsanitize=undefined, 5.0 added -fsanitize=leak), and build test/production binaries with it to run unit tests with
517 2017-01-13 22:59:53	0|morcos|Perhaps you are right that some of these new behaviors need to be more clearly documented
518 2017-01-13 23:00:30	0|sipa|cfields: there is also -fsanitize=thread, which cannot be used in conjunction with any of the others
519 2017-01-13 23:00:44	0|morcos|I'm not sure about listing the outputs in listunspent though...   hmm..  Will take a look at any more comments you have tomorrow
520 2017-01-13 23:02:38	0|sipa|cfields: downside is that both compilation and running is much slower
521 2017-01-13 23:03:30	0|bitcoin-git|[13bitcoin] 15kallewoof closed pull request #9235: [WIP] Refactor: Remove all uses of using namespace in all source files. (06master...06no-using-ns2) 02https://github.com/bitcoin/bitcoin/pull/9235
522 2017-01-13 23:10:02	0|BlueMatt|morcos: I'm not sure about listing either, thats why my first request was to just document it