--- Log opened Mon Aug 31 00:00:04 2020 00:32 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Ping timeout: 244 seconds] 01:00 -!- Netsplit *.net <-> *.split quits: cloudhead, titanbiscuit, CubicEarth 01:05 -!- titanbiscuit [~tbisk@66.115.154.150] has joined #rust-bitcoin 01:05 -!- CubicEarth [~CubicEart@c-67-168-1-172.hsd1.wa.comcast.net] has joined #rust-bitcoin 01:05 -!- cloudhead [~cloudhead@li1811-8.members.linode.com] has joined #rust-bitcoin 01:26 -!- yancy [~root@li1543-67.members.linode.com] has joined #rust-bitcoin 01:46 -!- jonatack [~jon@213.152.161.40] has joined #rust-bitcoin 04:20 -!- jonatack [~jon@213.152.161.40] has quit [Ping timeout: 240 seconds] 05:13 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #rust-bitcoin 07:18 -!- Maggie45Prosacco [~Maggie45P@static.57.1.216.95.clients.your-server.de] has joined #rust-bitcoin 07:28 -!- gribble [~gribble@unaffiliated/nanotube/bot/gribble] has quit [Remote host closed the connection] 07:33 < BlueMatt> ariard: yea, i think we should def improve the testing to ensure that with/without filtering everything works the same 07:34 < BlueMatt> ariard: will try to do a full review of 649 today. 07:37 -!- Netsplit *.net <-> *.split quits: simian_za, wraithm 07:39 -!- Netsplit over, joins: wraithm 07:40 -!- simian_za [~simian_za@195.159.29.126] has joined #rust-bitcoin 07:43 -!- gribble [~gribble@unaffiliated/nanotube/bot/gribble] has joined #rust-bitcoin 08:27 < BlueMatt> oh, after jkczyz pushes his updates, i guess 08:55 < jkczyz> yup, will do so today 09:05 < BlueMatt> andytoshi: are you gonna get a chance to bump rust-bitcoin this week? 09:13 < cloudhead> hey how was the `Params::pow_limit` meant to be used when validating the block target? problem is `BlockHeader::pow_validate(target)` takes a target that is compared with the header target, which means it has only 32 bits of precision, but `pow_limit` has 256 bits of precision, so it has to be changed to actually check a block against the min difficulty.. 09:14 < cloudhead> to do this currently I convert it back and forth, like target -> bits -> target 09:14 < cloudhead> but it seems inefficient 09:14 < cloudhead> is there a better way? 09:15 < cloudhead> I guess having a `pow_limit_bits` in `Params` would save one conversion 09:19 < BlueMatt> cloudhead: BlockHeader::validate_pow() takes the full u256 target. 09:20 < BlueMatt> cloudhead: so you can { let t = BlockHeader::target(); if t > (?) Params::pow_limit { t = Params::pow_limit; }; header.validate_pow(t); } 09:21 < BlueMatt> or something like that 09:21 < BlueMatt> I'd need to think hard if its > or < there, tho 09:24 < cloudhead> BlueMatt: as far as I can tell that won't work, because internally, validate_pow does `self.target() == required_target`, but self.target() only has 32 bits of precision, whereas Params::pow_limit has 256 bits 09:24 < cloudhead> they are both 256 bit integers, but when the target is extracted from the header bits, it necessarily only has 32 bits of entropy 09:24 < cloudhead> so the last N bits are 00000000. 09:24 < cloudhead> vs. pow_limit which has them as ffffffff 09:25 < cloudhead> so it could be that BlockHeader::target is wrong, if you think this should work 09:25 < cloudhead> ie. it should have Fs instead of 0s 09:26 < cloudhead> so for eg. a min difficulty header will have a target of ~ 0x1d00ffff0000000000000000000..., whereas pow_limit == 0x1d00ffffffffffffffffffffffffff 09:31 < BlueMatt> cloudhead: afair, it should be fine since target() isn't a simple 32 bit integer, there's an awkward algorithm to extract it as a float-like thing into a pow_limit 09:32 < BlueMatt> cloudhead: and min_target must be expressable as a u256 target 09:32 < BlueMatt> cause, otherwise you cannot mine a min-target block 09:33 < cloudhead> hmm but if you call target(), you will never get pow_limit 09:33 < cloudhead> because the bits after the 32bits are zeroed out 09:37 < BlueMatt> hmm, at least looking at the source i think maybe pow_limit() isn't a consensus value but a utility value - it is set to 0x00000000ffffffffffff... 09:37 < BlueMatt> not 1d00f.... 09:40 < BlueMatt> though it does seem bogus that validate_pow() requires you to pass in a u256 that it just recalculates if you used .target(), indeed 09:40 < cloudhead> yeah sorry, pow_limit when converted to 32 bits gives you the correct 0x1d00ffff 09:40 < BlueMatt> that can probably be fixed, though its not a huge deal given the sha256 will be much slower 09:40 < cloudhead> but since validate_pow() doesn't convert to bits, that doesn't work 09:40 < BlueMatt> hmm? i mean the code is fine 09:40 < BlueMatt> its just a bit slow for no reason in a common use-case 09:41 < cloudhead> well what are you supposed to pass as min-difficulty to validate_pow? 09:41 < cloudhead> that's what I'm trying to understand 09:41 < cloudhead> because pow_limit as-is will not work 09:41 < cloudhead> or are you saying the constant is just missing 09:42 < BlueMatt> right, i guess call header.target() on the genesis block? but, yes, the correct constant here is just missing 09:42 < BlueMatt> I would agree that pow_limit *should* be changed to be the correct constant 09:42 < BlueMatt> but it currently is not 09:42 < cloudhead> because if target() is correct, then the min-difficulty should be something like 0x000000ffff0000000000000000000000 if I'm not wrong 09:42 < cloudhead> using the genesis block is a good idea actually 09:43 < cloudhead> ok 09:43 < BlueMatt> i believe that is the correct *actual* min target, yes, the one in Params is just a bogus pretty-close approximation 09:43 < cloudhead> ok cool 09:43 < BlueMatt> which is good enough if you arent validating it 09:43 < cloudhead> right right 09:44 < cloudhead> ok thanks 09:44 < BlueMatt> also, for some reason, it looks like blockdata::constants::max_target() is correct 09:44 < cloudhead> hmmm true 09:45 < BlueMatt> anyway, let me go open a pr to fix the params ones 09:45 < cloudhead> though only for Mainnet 09:45 < BlueMatt> well, and testnet, but, yes 09:45 < cloudhead> right, yes! 09:46 < cloudhead> BlueMatt: awesome 09:46 < cloudhead> thought I'd bring this up so it could perhaps get in before the release 09:46 < BlueMatt> yea, thanks 09:48 -!- Maggie45Prosacco [~Maggie45P@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 265 seconds] 09:49 < BlueMatt> cloudhead: right, ok, now i see where it came from. those constants are set the same as powLimit in Bitcoin Core, which is what is compared against the targets, *but* powLimit is effectively always used by first converting it to the 32-bit float-ish "Compact" representation with only < or > used. 09:49 < BlueMatt> so...its confusing 09:59 < cloudhead> ah I see 10:00 < cloudhead> yes when converted to compact, it's the correct number, but that's not so useful in the context of validate_pow 10:00 < cloudhead> so I wonder what's best thing to do 10:02 < BlueMatt> a) document, document, document. b) probably change it to the to-bits-and-back version, though needs careful review, specifically in core's consensus its compared to before being converted to compact and back, so in order to do the conversion I think we need to be absolutely convinced that the to-bits-and-back version is the smallest possible value which is >= powLimit. 10:08 < cloudhead> I see I see 10:08 < cloudhead> yeah, intuitively it seems like for < and > it may not make a difference 10:08 < cloudhead> it's only the equality check that breaks 10:08 < cloudhead> but I may be wrong 10:08 < BlueMatt> right, but its a float-like thing.....and floats have a tendency to surprise :/ 10:08 < cloudhead> yeah hehe 10:09 < cloudhead> another option is to provide in params a 32bit version 10:10 < cloudhead> and then the user can rely on converting it to the target when needed 10:10 < cloudhead> this is what btcd does afaict 10:10 < BlueMatt> well you can always convert back and forth. 10:11 < cloudhead> https://github.com/btcsuite/btcd/blob/master/chaincfg/params.go#L241-L242 10:11 < cloudhead> yes true 10:11 < cloudhead> easy to mess up though if you're not aware of this 10:12 < BlueMatt> right, just exposing more versions makes it very easy to accidentally break your validation in corner-cases 10:12 < cloudhead> yeah 11:26 -!- shesek [~shesek@164.90.217.137] has joined #rust-bitcoin 11:26 -!- shesek [~shesek@164.90.217.137] has quit [Changing host] 11:26 -!- shesek [~shesek@unaffiliated/shesek] has joined #rust-bitcoin 11:29 -!- endangurura [~endanguru@unaffiliated/endangurura] has joined #rust-bitcoin 11:33 < BlueMatt> cloudhead: this better? https://github.com/rust-bitcoin/rust-bitcoin/pull/464 :) 11:56 -!- endangurura [~endanguru@unaffiliated/endangurura] has quit [Quit: WeeChat 2.8] 12:03 < cloudhead> BlueMatt: yes, perfect, nice rationale also, makes sense :) 12:37 < ariard> BlueMatt: updated #633 with new commit for ChannelKeys 12:38 < ariard> BlueMatt: what's the state of #618? I should look on it before it's merged I guess? 12:39 < ariard> lmk if there are other PRs which need pending reviews (MPP on the router side?) 12:39 < BlueMatt> ariard: thanks, will look again at 633. re: 618 I still need to respond to some questions from val, but its mostly read to go. its blocked waiting on a rust-bitcoin release, which should be soon, but I'm not in a massive rush to hit merge on that one...its not gonna conflict with anything, and there's still downstream work ongoing that may result in a wishlist for 618 12:41 < BlueMatt> ariard: you're def welcome to look at 646, but anchor and #630 is also useful work to be doing :) 12:41 < BlueMatt> or, of course, anything that got merged while you were away. 12:42 < BlueMatt> eyeballs always good, even post-merge. 12:55 < ariard> BlueMatt: okay will review #646 and then anchor :) 12:55 < ariard> I'll address #580 and few other holes we still have like preimage passing-backward IIRC 12:57 < ariard> Okay #653 is a fix but needs test coverage, I think in fact it might be addressed by #649 12:58 < BlueMatt> ariard: oh, shit, sorry, somehow 653 fell off my radar 12:58 < BlueMatt> ariard: does it make sense to write a test for it and then drop the commit in favor of 649? 13:08 < ariard> BlueMatt: yeah I was thinking about dropping it, will propose a test on #649 directly 13:11 < BlueMatt> thanks. 13:11 < BlueMatt> sorry about that...not sure why it dropped off my radar :( 13:14 < ariard> BlueMatt: no worries, my fault I've to much PRs half-alive :p 13:14 < ariard> BlueMatt: btw meeting 14:16 < notmandatory> stevenroose: hi, jrawsthorne has a change ready to upgrade murmel to use the latest version of rust-bitcoin, can you take a look? we need this in so he can finish his PR to upgrading murmel to also use the lastest rust-bitcoin, thanks!.. for anyone else interested the PR is: https://github.com/rust-bitcoin/hammersbald/pull/13 14:41 < stevenroose> notmandatory: sure, can do! I haven't been able to get much rust-bitcoin work done the last 2 months :/ 14:46 < notmandatory> stevenroose: no prob, sorry to bug you.. I'll just raise murmel related stuff to you here after they've had some review I think they're ready to go :-) 14:48 -!- DeanWeen [~dean@gateway/tor-sasl/deanguss] has quit [Remote host closed the connection] 16:34 < notmandatory> stevenroose: btw.. above PR is for hammersbald.. but is a prerequisite for his murmel PR 18:02 -!- DeanWeen [~dean@gateway/tor-sasl/deanguss] has joined #rust-bitcoin 22:54 -!- shesek [~shesek@unaffiliated/shesek] has quit [Remote host closed the connection] --- Log closed Tue Sep 01 00:00:05 2020