1 2015-07-21 00:02:09 <dgenr8> probably worth doing incrementally usually, since most txes in blocks won't affect others in mempool, and mempool might be 300x the size of a block
  2 2015-07-21 09:41:16 <jtimon> sipa morcos petertodd any thoughts on the latest #6448 intended to make -maxmempool=0 ok? I still need to fix some tests since as a side effect some error strings change
  3 2015-07-21 10:46:00 <jtimon> cfields: Luke-Jr can you find 3 minutes to review #6068 ? at this point it is really a trivial review
  4 2015-07-21 11:48:35 <jtimon> It would be nice to have a C libconsensus that can be proved to satisfy its specifications...
  5 2015-07-21 11:48:35 <jtimon> mhmm, I didn't knew you can automatically prove bubble-sort's correctness with just 16 annotations...https://en.wikipedia.org/wiki/Frama-C
  6 2015-07-21 11:49:26 <wumpus> proving that it is equal to the C++ implementation will be the hard part
  7 2015-07-21 11:49:50 <jtimon> of course we need a complete libconsensus first just thinking out loud
  8 2015-07-21 11:50:17 <jtimon> well, wumpus I was assuming that the C++ implementation slowly evolved to be C only
  9 2015-07-21 11:50:20 <wumpus> it's not that difficult to port libconsensus to C, but there's no guarantee it will be exactly the same, it's no different from re-implementing that other node implementations have done
 10 2015-07-21 11:50:35 <wumpus> with the hardfork risks of that
 11 2015-07-21 11:51:01 <wumpus> the whole reason of libconsensus is to be able to use bitcoin core's consensus code -as is- in other node impls
 12 2015-07-21 11:51:45 <wumpus> that's also why we should be so careful with any refactoring needed for libconsensus - in the process of getting there we may break the whole reason it makes sense to have it
 13 2015-07-21 11:52:41 <jtimon> well, I think the main risk is when you change the compiler, I don't think changing a boost_foreach for a regular fork or turning a method into a regular function has that much of a hardfork risk
 14 2015-07-21 11:53:37 <wumpus> I don't *think* so either. But it requires only a very minor mistake.
 15 2015-07-21 11:53:39 <jtimon> wumpus: but yes, I'm perfectly aware that we should be careful
 16 2015-07-21 11:53:50 <wumpus> and no one is perfect
 17 2015-07-21 11:54:16 <jtimon> unfortunately that imposes that the changes need to be small and incremental and unfortunately that means very few reviewers give a shit
 18 2015-07-21 11:54:42 <wumpus> they may give a shit, but not trust themselves enough to get it right
 19 2015-07-21 11:55:31 <jtimon> but now that we're talking about it, can we please decide on the moveonly? https://github.com/bitcoin/bitcoin/pull/6051#issuecomment-120708121
 20 2015-07-21 11:56:19 <wumpus> I mean it has happened before that seemingly trivial refactors in the consensus code caused (potential) hardfork risks, only caught at the last minute
 21 2015-07-21 11:57:16 <wumpus> #6051 is closed?
 22 2015-07-21 11:57:46 <jtimon> wumpus: well, if it was lack of trust they would read my code without trusting me or at least nack it on the grounds of "too complex to review for a consensus refactor" or something
 23 2015-07-21 11:58:18 <jtimon> wumpus, yes, is closed and the reason is explained in the comment on the link
 24 2015-07-21 11:58:24 <wumpus> don't get me wrong, it's not about trust as in 'afraid that you're trying to break the system deliberately'
 25 2015-07-21 11:58:58 <wumpus> just that these things are extremely hard to get right, there's no overstating that
 26 2015-07-21 11:59:53 <jtimon> yeah yeah, it's as in "I'm afraid you will make a mistake and break the system" I get that, I was only pointing out that only a reduced number of reviewers seem interested in reviewing incremental consensus refactors, not even to look for those mistake I may make
 27 2015-07-21 11:59:55 <wumpus> I only see a "closing for now"
 28 2015-07-21 12:00:27 <jtimon> please follow the link I just posted
 29 2015-07-21 12:00:41 <jtimon> it directs you to the comment right before that
 30 2015-07-21 12:00:54 <wumpus> I don't see anything there about closing the issue
 31 2015-07-21 12:01:05 <jtimon> Do we care about being able to expose VerifyHeader without being prepared to also expose VerifyTx and VerifyBlock?
 32 2015-07-21 12:01:05 <jtimon> Do we care about being able to expose VerifyTx without being prepared to also expose VerifyHeader and VerifyBlock?
 33 2015-07-21 12:01:05 <jtimon> If we don't care about exposing verifyTx or verifyHeader before verifyBlock is ready, then we can just have policy.cpp and we can move more things at the same time (CheckBlock, ContextualCheckBlock and GetBlockSubsidy, for example).
 34 2015-07-21 12:01:05 <jtimon> I'm having second thoughts about txverify.cpp + blockverify.cpp vs policy.cpp.
 35 2015-07-21 12:01:05 <jtimon> Thoughts?
 36 2015-07-21 12:01:13 <wumpus> you don't have to post the entire post hee
 37 2015-07-21 12:01:35 <jtimon> yeah, I just posted the link
 38 2015-07-21 12:01:49 <wumpus> but how does it explain why you closed the issue?
 39 2015-07-21 12:01:54 <wumpus> I honestly don't get it
 40 2015-07-21 12:01:58 <wumpus> I'm not trying to troll you or so
 41 2015-07-21 12:02:06 <jtimon> I'm closing it until I get some feedback on that
 42 2015-07-21 12:02:28 <wumpus> okay then, clear, why not say that directly
 43 2015-07-21 12:02:35 <jtimon> I know people like gmaxwell don't like frequent code movements
 44 2015-07-21 12:03:01 <wumpus> well it probably breaks all the other work in progress, eg the mempool management
 45 2015-07-21 12:03:05 <jtimon> wumpus: I tried to say that, but my communication skills are not as good as I would like them to be
 46 2015-07-21 12:03:27 <wumpus> though not sure, it shouldn't involve many consensus changes
 47 2015-07-21 12:03:44 <wumpus> also this would be a one-time thing right?
 48 2015-07-21 12:03:50 <wumpus> not "frequent"
 49 2015-07-21 12:03:59 <wumpus> jtimon: ok, no problem
 50 2015-07-21 12:04:37 <jtimon> well, it probably doesn't break anything mempool related: it's move only and the mempool changes don't touch consensus (well, my proposal does)
 51 2015-07-21 12:05:26 <jtimon> any thoughts on my no-mempool code draft?
 52 2015-07-21 12:05:54 <jtimon> (#6448 )
 53 2015-07-21 12:06:30 <wumpus> not at the moment - also still need to review sipa's mempool changes
 54 2015-07-21 12:07:00 <jtimon> it has a dumb replacement policy (sipa's smater one rebased on top for later), but the goal in mine is that 0 size for the mempool is fine
 55 2015-07-21 12:07:03 <wumpus> it is very difficult and tricky code for me
 56 2015-07-21 12:07:22 <jtimon> which one?
 57 2015-07-21 12:07:28 <wumpus> the mempool mangement in general
 58 2015-07-21 12:07:52 <jtimon> well, that's because acceptToMemoryPool really needs a serious cleanup
 59 2015-07-21 12:08:22 <jtimon> which, btw, #6448 requires and does
 60 2015-07-21 12:08:24 <wumpus> it's not the organization that is difficult, but the logic, very hard to see if it is correct - I don't think a 'cleanup' would change that much, I'm used to disordered code that's not the problem :-)
 61 2015-07-21 12:09:49 <jtimon> well, the coe around relay fees and free transactions is 10 times more complicated than it needs to be, so yes, I think the organization makes reasoning about the logic much harder
 62 2015-07-21 12:11:31 <jtimon> at least to me, it is much simpler to think about that code after commits like https://github.com/jtimon/bitcoin/commit/4db612d3f828d13be362e49313668cbcb9e3b4a3 or https://github.com/jtimon/bitcoin/commit/abbcdb1063870402a9e23d5f7cd6f440d4827f5c (that I fear will be delayed forever as non-prioritary)
 63 2015-07-21 12:15:27 <jtimon> anyway does the general idea of a tx checks cache make sense? will that be enough to safely allow mempool-less nodes ?
 64 2015-07-21 12:16:45 <jouke> ,
 65 2015-07-21 12:17:04 <moa> what is the 'general idea of a tx checks cache'?
 66 2015-07-21 12:17:08 <jtimon> because if that idea doesn't make sense to you, you probably don't want to bother reading the rest of the code
 67 2015-07-21 12:18:24 <jtimon> moa: well, the code for that is https://github.com/jtimon/bitcoin/commit/935ee1ec875308f27339418363c787ec061d335f (the changes to main in that commit aren't really that hard to read)
 68 2015-07-21 12:18:38 <jtimon> let me try to explain it better with words...
 69 2015-07-21 12:19:00 <moa> well general idea implies you had something like that in mind
 70 2015-07-21 12:19:31 <jtimon> basically you have an in-memory map with txHash -> result of a previous check of the same transaction
 71 2015-07-21 12:19:42 <moa> ok
 72 2015-07-21 12:19:59 <jtimon> so before checking a transaction, you first make sure you haven't find it invalid already
 73 2015-07-21 12:20:58 <jtimon> with small mempools, this may be necessary or you can be forced to partially validate the same transactions many times
 74 2015-07-21 12:21:33 <jtimon> but it should be good for the unlimited mempool too
 75 2015-07-21 12:21:35 <moa> so you are replacing a mempool with txHash pool
 76 2015-07-21 12:22:03 <jtimon> no, complementing
 77 2015-07-21 12:22:21 <moa> complementing and minimising mempool
 78 2015-07-21 12:22:38 <jtimon> you may still want the mempool, for example for fee estimation (or because you're a miner)
 79 2015-07-21 12:23:12 <moa> is malleability a consideration?
 80 2015-07-21 12:24:15 <jtimon> well, the cache will consider malleated tx as different, but if they're invalid they will be rejected as normal
 81 2015-07-21 12:24:42 <jtimon> maybe malleability is one way to DoS attack the cache
 82 2015-07-21 12:25:27 <moa> would there be limits on cache size then?
 83 2015-07-21 12:26:07 <jtimon> yes https://github.com/jtimon/bitcoin/commit/935ee1ec875308f27339418363c787ec061d335f#diff-e5f4a67eb70ec7b0323f1913f57ffb12R30
 84 2015-07-21 12:26:55 <moa> any back of envelope on typical sizes ?
 85 2015-07-21 12:27:12 <moa> like XX TX == YY Mbyte
 86 2015-07-21 12:29:42 <jtimon> I took the 10000 max entries from a hat, but I was reasoning...each entry needs 32 bytes for the hash + 8 for the code and nDoS plus 1? for the bool and then there's a string (they tend to be short, but variable, this can be imroved later with a codeToString map)
 87 2015-07-21 12:31:38 <jtimon> so I was thinking on 50 something MBs (that can be improved later to as low as 33 bytes per entry [so 33 MBs] )
 88 2015-07-21 12:32:21 <jtimon> that is a huge cache though, maybe caching 1000 tx instead of 10000 is already more than enough
 89 2015-07-21 12:32:31 <moa> right
 90 2015-07-21 12:32:41 <jtimon> I don't know, that number would require some testing and feedback
 91 2015-07-21 12:32:50 <moa> ok, as a general idea I think it definitely has merit
 92 2015-07-21 12:32:55 <moa> imho
 93 2015-07-21 12:33:17 <jtimon> moa thank you
 94 2015-07-21 12:34:57 <jtimon> as said it also means that we can introduce the mempool limit even with a dumb replacement policy (ie, no replacement, once full the mempool doesn't get any more transactions), because rejected replacements can still be relayed if the only rejection reason was a full mempool
 95 2015-07-21 12:38:30 <moa> in some ways as network wide effect it would be hastening the inhomogeneity of the nodes, i.e. those running full mempools versus reduced, those running full blockchain versus pruned, etc
 96 2015-07-21 12:38:59 <moa> but that kind of fragmentation is going to be unavoidable I think
 97 2015-07-21 12:40:05 <wumpus> the network is already quite unhomogenous
 98 2015-07-21 12:40:20 <moa> inhomogeneous
 99 2015-07-21 12:41:00 <moa> :)
100 2015-07-21 12:43:22 <moa> so if ll nodes you connect to are running a reduced mempool is it possible that you could not get the actual transaction but only the hash?
101 2015-07-21 12:44:09 <moa> i.e. could you isolated by being surrounded by hash-only neighbours?
102 2015-07-21 12:44:18 <jtimon> moa: yeah that seems correct, but I'm not sure if that's necessarily a bad thing
103 2015-07-21 12:44:21 <wumpus> I suppose they wouldn't inv the transaction if they don't have it
104 2015-07-21 12:44:31 <jtimon> no-mempool nodes are still full nodes
105 2015-07-21 12:45:03 <wumpus> but I think mempool-limiting is better than having no mempool at all, at least the consequences are easier to understand
106 2015-07-21 12:45:21 <wumpus> (and even those aren't 100% clear)
107 2015-07-21 12:46:15 <jtimon> even if you have fully validate it once, you have to always check the inputs (not the scripts) since some of them could have been spent since the last time (also recalculating the fees and checking that again is good if we move to a dynamic relay fee)
108 2015-07-21 12:47:11 <jtimon> wumpus: this is not an alternative to limiting the mempool, it is actually a "necessary" step for deploying a limit with a dumb replacement policy
109 2015-07-21 12:47:45 <jtimon> you can just set a big default value as well (the original idea for #6448 )
110 2015-07-21 12:48:23 <wumpus> but #6455 achieves mempool limiting without doing that?
111 2015-07-21 12:48:31 <jtimon> but it seems to me that allowing small mempools can give the users problems even with the smartest replacement policy
112 2015-07-21 12:48:54 <wumpus> let's discourage that, then
113 2015-07-21 12:49:36 <wumpus> it's more important to have *a* bound on the mempool than a small bound, I mean the current nodes can run out of memory in case of a DoS, that is what needs to be avoided
114 2015-07-21 12:49:57 <jtimon> Oh, I was still looking at  #6421, but #6448 was originally a modified subset #6421
115 2015-07-21 12:50:15 <jtimon> why not fix it instead of discouraging it?
116 2015-07-21 12:50:27 <moa> txHash might be a candidate for swap?
117 2015-07-21 12:50:47 <jtimon> moa: I don't undesrtand the question
118 2015-07-21 12:50:49 <wumpus> because we need the least risky solution that works, on short term
119 2015-07-21 12:51:17 <wumpus> on longer term, with more deliberation, sure your idea makes sense
120 2015-07-21 12:51:22 <moa> say the mempool gets fill (RAM) and instead of dumoing them you put the hash on disk
121 2015-07-21 12:51:47 <wumpus> that changes the DoS problem from a fill-memory-up to a fill-disk-up
122 2015-07-21 12:52:24 <moa> disks are harder to fill
123 2015-07-21 12:52:36 <wumpus> it could be a good idea if you need ery large mempool sizes, but it doesn't avoid the limiting issue at all
124 2015-07-21 12:52:53 <moa> no but it does raise the fee bar
125 2015-07-21 12:52:56 <jtimon> wumpus: I think #6448 (+441 −171) is simpler than #6455 (+534 −154)
126 2015-07-21 12:53:51 <wumpus> e.g. the mempool size only grew over ~20MB with the DoS attack, that hardly warrants writing anything to disk
127 2015-07-21 12:53:52 <sipa> jtimon: i don't think we have agreement on how the short and long term mempool limitation ideas will work... i think it's too early to try to generalize them
128 2015-07-21 12:54:30 <moa> wumpus: i agree but it is useful to have a bazooka back-up plan
129 2015-07-21 12:54:57 <wumpus> moa: but implementing unnecessary things will just burden the code, make it harder to test, etc
130 2015-07-21 12:55:06 <jtimon> sipa: my solution doesn't assume a smart mempool replacement policy and it's leaving that part for later (well, just rebased what you had on  #6421 on top of it)
131 2015-07-21 12:56:19 <sipa> jtimon: i know, but just limiting the mempool without any further changes now isn't what we want
132 2015-07-21 12:56:32 <jtimon> wumpus: sipa: I think we should have merged a dumb limit with a huge default already, it would make anything on top of that much easier to propose and review
133 2015-07-21 12:56:41 <sipa> so the refactor may make sense, depending on what else gets implemented
134 2015-07-21 12:57:34 <sipa> but it's not like we want any released code to actually just limit mempool and stop...
135 2015-07-21 12:57:49 <wumpus> right
136 2015-07-21 12:57:59 <jtimon> sipa: I do further changes: the idea is to make no-mempool nodes secure, which I think it's easier than a smart mempool replacement possible
137 2015-07-21 12:58:12 <jtimon> s/possible/policy
138 2015-07-21 12:59:19 <sipa> jtimon: i understand you want the code and the history to be clean, but tyere are too many ideas about short term and long term changes... not all implemented, andbfewer tested
139 2015-07-21 12:59:54 <sipa> i really want to first focus on getting something to work
140 2015-07-21 13:00:19 <wumpus> +1 ipa
141 2015-07-21 13:01:08 <jtimon> not just that, you want to focus on getting the smart replacement policy to work, you don't think the cache is a viable solution
142 2015-07-21 13:01:56 <jtimon> please say so clearly if that's the case, nack is always better than silence
143 2015-07-21 13:02:04 <sipa> i agree that a clean history makes sense, but making changes that break things, without knowing what will go on top is bad (i don't think a dumb limit is we want in any release)
144 2015-07-21 13:02:33 <sipa> well, petertodd and morcos have been discussing various changes
145 2015-07-21 13:02:41 <jtimon> forget about the clean history please, I wish I had never said that
146 2015-07-21 13:02:45 <sipa> but it sounds like it is really hard problem
147 2015-07-21 13:03:22 <sipa> maybe the long term solution is a really different style mempool, or no mempool at all, and a suggestion to change the p2p protocol
148 2015-07-21 13:03:44 <jtimon> well, my really simple cache solution solves the biggest part of the problem I think
149 2015-07-21 13:04:03 <sipa> i have missed that
150 2015-07-21 13:04:10 <sipa> what do you cache?
151 2015-07-21 13:04:26 <jtimon> we can calmly work on the really hard smart replacement fee problem later
152 2015-07-21 13:05:12 <jtimon> rejection reasons for transaction hashes
153 2015-07-21 13:06:09 <jtimon> it can be improved, but I really think it is a very simple and revieable patch
154 2015-07-21 13:06:29 <sipa> petertodd already has a rejection cache patch
155 2015-07-21 13:06:35 <sipa> using bloom filters
156 2015-07-21 13:06:42 <jtimon> what I was talking about with moa just before you entered the discussion
157 2015-07-21 13:07:55 <jtimon> well, mine actually also catches fullSuccessOnce (please don't assume I'm using it stupidly)
158 2015-07-21 13:08:02 <jtimon> where's petertodd's patch?
159 2015-07-21 13:08:40 <jtimon> I can't believe I missed it in his no-mempool mail
160 2015-07-21 13:08:44 <sipa> #6452
161 2015-07-21 13:09:10 <sipa> it's not a solution for no-mempool
162 2015-07-21 13:09:18 <petertodd> wumpus: actually the maximum the mempool grew to during the attack was well over 20MB, IIRC more like 100MB+
163 2015-07-21 13:10:19 <jtimon> oh, petertodd you actually didn't posted the code in the ml...
164 2015-07-21 13:10:38 <moa> petertodd: that size is dependent upon definition of a 'TX'
165 2015-07-21 13:11:33 <sipa> petertodd: mempool grew to 150 MByte tx size iirc, and over 300 Mbyte ram usage
166 2015-07-21 13:11:43 <petertodd> moa: I'm talking about blockchain size
167 2015-07-21 13:12:28 <morcos> sipa: i think you agree 6455 shouldn't be merged as is?  i don't want to sabotage moving forward on this, but any objection to me commenting on the pull, that its more appropriate for concept review at this point than full testing?
168 2015-07-21 13:12:34 <moa> the blockchain is over 40GByte?
169 2015-07-21 13:12:41 <petertodd> jtimon: why would I? avoiding bw wasting by not asking for tx's twice isn't a change that needs discussion on the mailing list, just a simple optimization
170 2015-07-21 13:13:13 <petertodd> moa: as in, size of the tx when it's in the blockchain
171 2015-07-21 13:13:15 <jtimon> mhmm next time you vaguely propose something you have implemented...please think that other people may not know about it and may want to go and try implement it themselves
172 2015-07-21 13:13:39 <petertodd> jtimon: huh? don't you look at pull-reqs?
173 2015-07-21 13:13:46 <moa> ah so maximum mempool in terms of confirmed TX
174 2015-07-21 13:14:14 <jtimon> petertodd: I don't always look at all PRs, no, I'm sure nobody has time for that
175 2015-07-21 13:14:43 <petertodd> jtimon: #6452 was about 2-3 hours of work to write the first draft of anyway, as marcos did an initial draft using limitedmap's rather than bloom filters
176 2015-07-21 13:14:55 <morcos> jtimon: i mentioned the concept on IRC some time ago, created a PR when I got no response, and then petertodd improved on it.  but i'm not sure it solves the same thing as what you're attempting
177 2015-07-21 13:15:14 <petertodd> jtimon: well, I'd suggest you start then... there's really not very many PR's, particularly if you ignore ones unrelated to stuff you're not interested in (e.g. gui in my case)
178 2015-07-21 13:15:37 <morcos> o, my friend, o.  morcos.  :)
179 2015-07-21 13:15:41 <sipa> morcos: 6455 is more intended as a testable combined solutiom than as something intended to be merged
180 2015-07-21 13:15:42 <jtimon> I mean, no big deal because I learned doing it (even though I didn't thought about caching the rejectments where you're doing it, which seems to make more sense)
181 2015-07-21 13:16:20 <sipa> morcos: and the problems with clogging up the bottom preventing entry seem serious, if found testable
182 2015-07-21 13:16:23 <petertodd> jtimon: sounds like you're doing something very different anyway
183 2015-07-21 13:16:57 <sipa> morcos: do you think it should be merged as-is?
184 2015-07-21 13:17:12 <morcos> sipa: thats what i thought, i just saw wumpus mentioned he wanted to review it, which is fine, but just wanted people to understand it had issues
185 2015-07-21 13:17:16 <morcos> sipa: no definitely not
186 2015-07-21 13:17:25 <jtimon> petertodd: well, I'm not using bloom filters, just the tx hash