< kallewoof>
I'll look into making simple tests that check the current signet consensus stuff with what we have, if people are uncomfortable with the PR not having enoguh tests, but i personally think "worst thing that happens is that signet does not work" (everything else has tests, after all, and the signet code *is* touched then too) fwiw.
< kallewoof>
wumpus sipa aj ^
< sipa>
kallewoof: what PoW difficulty does (the default) signet have?
< kallewoof>
sipa: very low, lower than main/testnet
< sipa>
or maybe better, how low can you set it for a custom one
< kallewoof>
it takes a few seconds on a cpu initially
< aj>
kallewoof: poking at making it accept OP_TRUE as the block challenge with no witness commitment, so plain -generate works
< sipa>
alternatively, if it can be made low enough, a block could be constructed entirely on the python side (the test framework probably has nearly everything already)
< kallewoof>
we could also just pre-mine the block(s) in that case. if the coinbase etc. are all predefined we can set the nonce and such. shouldn't be needed tho as the difficulty is pretty low. (the original mining tests used the c++ miner though, so it may be slow actually..)
< sipa>
yeah, that's also a possibility
< sipa>
mining from the python side means you could easily extend it with some less trivial challenge script too (though that would mean adding the sighash logic there as well, which may make sense anyway)
< kallewoof>
yeah, i mean the signing code is not big; we could literally drop the generate script into the test framework and use it
< kallewoof>
aj: that would mean the generate script can import those things without doing import path hacks too.. maybe the contrib/signet stuff should just call ../../test/functional/test_framework/(signet_)generate.py or something
< sipa>
kallewoof: fwiw, the powLimit in the signet PR doesn't exactly match the difficulty as specified in BIP325 (the digits after 00002adc28 should be zeroes)
< aj>
kallewoof: another option (short of including generate.py) might be have the blockscript be "OP_SHA256 x OP_EQUAL" which would mean just adding a constant suffix to the witness commitment for each block
< sipa>
exercising the sighash logic would be even nicer :)
< * sipa>
hides
< kallewoof>
sipa: are you saying that the compact 0x1e2adc28 expands to 00002adc28000[...] and not the one that is there now? I derived it the other way around, and just kept it as is. Having zeroes makes sense but would mean another reset.
< sipa>
kallewoof: yes
< sipa>
well it wouldn't invalidate any existing blocks (with high probability), but it is technically a signet softfork i guess
< kallewoof>
ohh... actually, you're right it wouldn't. it would just make the limit drop a small amount
< sipa>
i mean: the implementation doesn't match the spec right now (and though the probability is small, it's not unobservable; one in 299928 attempts)
< sipa>
one in 299928 *succesful* attempts
< aj>
sipa: yeah, that needs generate.py or equivalent
< kallewoof>
aj: generate.py without the copy-pasted stuff is not large, though. maybe it should (permanently) reside in the test framework path
< kallewoof>
but that means more for reviewers to review, so probably just try to hack something easy for now..
< aj>
kallewoof: yeah, maybe; don't really want to add it in before cleaning it up more though
< kallewoof>
aj: all right
< aj>
sipa, kallewoof: changing the powLimit to 2adc280000000 doesn't seem to invalidate any existing blocks
< kallewoof>
i just confirmed that it doesn't, too
< kallewoof>
pushed squashie
< kallewoof>
is there a point not squashing if you end up having to rebase on master due to a conflict? I can't see one personally, so gonna squash.
< gwillen>
kallewoof: if you squash + rebase in one operation it becomes slightly harder to tell whether the squash was just a squash or changed anything (when re-reviewing), but I don't know that it's a big deal
< gwillen>
unless it's a large complex PR with a bunch of existing review
< gwillen>
(and you can always use commandline git to check)
< gwillen>
(but it's a bit of a PITA)
< kallewoof>
gwillen: i made squash commits, then ended up with a conflict against master and rebased. the entire PR is as such "detached" from whatever commit reviewers saw the last time, so i could've added stuff to the other commits without anyone noticing at that point.
< sipa>
kallewoof: unless PRs are huge (i haven't reviewed your signet one on detail, but it doesn't look very large), i find that most of the work in a review is understanding the flow of changes and how things fot together
< kallewoof>
yeah, i thought so, but i caused poor jonatack to have to restart his review several times cause i kept squashing changes.
< sipa>
once that's done, i don't mind reviewing things from scratch again... it'll go a lot faster
< sipa>
and for the fact that you coukd introduce small bur dangerous changes that could be missed on re-review... well ideally that's what you have tests for
< kallewoof>
*nod*
< sipa>
they don't just help guarantee that functionality won't break, they also give reviewers confidence
< kallewoof>
yeah, tests are important. i felt like i had them in the signet PR because i USED to have them, before I split it up to make review easier.
< sipa>
kallewoof: i think i've moved in that direction as well; for segwit, everything was tested by adding segwit support into the wallet and using that to test the consensus logic... which i think made the change much bigger than it needed to be
< gwillen>
I don't know if this is something we could add to one of the workflow docs under doc/, but it's not too hard to ask git to check for you whether squashing changed anything
< sipa>
for taproot there is zero wallet support, and signing isn't even implemented in the consensus pr... but there are tests in python that just reimplement it
< gwillen>
it's a bit fiddly, but now that github allows checking out arbitrary commits, including ones that were rebased-away, it's more straightforward
< kallewoof>
sipa: yeah, i noticed! it makes sense, but i was surprised when i was trying to use it in btcdeb/tap :)
< sipa>
kallewoof: right, i see how that complicates things :)
< kallewoof>
gwillen: i am actually confused why git(hub) doesn't have a way to just compare changes since last time. like, give me a diff of what i reviewed before and now
< kallewoof>
gwillen: i hit that "submit review" button so github clearly knows exactly where i was the last time..
< gwillen>
I know that gitlab makes this possible, I thought github had some way to do this
< gwillen>
but at least in gitlab, the only thing you can compare is "changes since last push", so if they rebase against changes to upstream, it's useless because the diff includes all the upstream changes too
< kallewoof>
yeah... having a snapshot of the state when you last reviewed and comparing that to current form would be ideal
< sipa>
i think there is just less effort done on making rebase-heavy workflows easy
< gwillen>
at least when it says "so-and-so force-pushed from <id> to <id>", getting a direct diff on the command line is easy
< gwillen>
just 'git fetch <id>' each of them, then 'git diff <id> <id>'
< aj>
i think lots of rebases is something a few open source projects do, but most/paying customers tend not to?
< gwillen>
(however if that includes a rebase against new upstream changes, that command won't be useful because it will include those, the right command for that is a little more subtle)
< luke-jr>
sipa: when you're splitting up the Taproot PR, I do think it would make sense to do consensus and policy separately
< gwillen>
ahhh, I always forget this -- in the github UI when you see "so-and-so force-pushed", the words "force-pushed" are a link to the diff
< gwillen>
they kind of hide it, but if you click there you can see the changes in the force push
< luke-jr>
gwillen: the notification emails have links that I suspect are supposed to be that, but they never seem to work for me
< aj>
kallewoof, sipa: so currently signet/generate.py supports signing the blocktx via either constructing a psbt, or by calling signrawtransactionwithwallet directly. any reason to keep the latter or can/should i go psbt only, do you think?
< MarcoFalke>
gwillen: Are you aware of git range-diff ? This helps re-reviewing a fixed up version of the changes, and it "hides" any rebases that happened in between.
< sipa>
MarcoFalke: TIL
< MarcoFalke>
Well, it can also be used to review rebases and see what manual changes were needed to rebase.
< aj>
oops, looking at rust-bitcoin and misread "macro_rules!" as "marco_rules!"
< MarcoFalke>
Though, it doesn't work for large rewrites... In which case you are better of reviewing from scratch anyway
< MarcoFalke>
macro fake
< sipa>
in c++ most uses of marcos have been replaced with templates, thankfully
< vasild>
Has it been considered before importing an external library into bitcoin core as a git submodule?
< vasild>
I am trying to add a stripped version of crypto++ - just the bits that do sha3-256, but it is all tied together by internal dependencies, so the process may end up importing e.g. 80% of the library source
< MarcoFalke>
Which dependency is tor using?
< vasild>
let me see...
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #19826: Pass mempool reference to chainstate constructor (master...2008-valMemRef) https://github.com/bitcoin/bitcoin/pull/19826
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #19827: init: Disallow negation of blockversion (master...2008-parseBoolBlockVersion) https://github.com/bitcoin/bitcoin/pull/19827
< wumpus>
vasild: I woudl suggest finding a stand-alone implementation of SHA3, a reminder: it does not need to be efficient
< wumpus>
we're not going to take up a subtree of submodule just for this
< wumpus>
MarcoFalke: exactly, a tiny implementation (as far as possible for something like Keccak) is preferred here
< vasild>
I gave up with crypto++ after it turned out that it is not possible to take just sha3.h and sha3.cpp from its implementation (I don't want to do any modificatoins of those imported files)
< wumpus>
sure, I understand you don't want to end up maintaining this, but I wouldn't say small modifications (e.g. type names) are a problem to fit it better in our repository
< wumpus>
nice
< vasild>
I also added a wrapper class SHA3_256 that has the same interface as our existing hashers: Write(), Finalize() and Reset() methods.
< wumpus>
awesome, it's good to be consistent about that
< vasild>
yeah, would also help if we decide to ditch whatever we have imported and import something else
< fanquake>
That certainly looks better than random Tor code, git submodules, OpenSSL or other shared libs
< wumpus>
exactly
< vasild>
:-)
< wumpus>
we were getting scared for a bit
< vasild>
fear not! :-D
< kallewoof>
Weirdly, one of my linux machines complains that ./optional.h:24 ('static auto& nullopt = boost::none;') is unused in like 40+ places; it looks correct, but none of my other machines give me that warning no matter what flags I try (the specified -Wunused-variable is already present, I think)...
< wumpus>
isn't *const* missing?
< wumpus>
or constexpr
< wumpus>
it doesn't look right if you can assign something else to nullopt, and also, this is likely the reason you get a warning when including the file and not using nullopt
< bitcoin-git>
bitcoin/master aba0335 John Newbery: [net processing] Remove CNodeState.name
< bitcoin-git>
bitcoin/master 7cd4159 John Newbery: [net processing] Add Peer
< bitcoin-git>
bitcoin/master 1f96d2e John Newbery: [net processing] Move misbehavior tracking state to Peer
< bitcoin-git>
[bitcoin] laanwj merged pull request #19607: [p2p] Add Peer struct for per-peer data in net processing (master...2020-07-peer) https://github.com/bitcoin/bitcoin/pull/19607
< meshcollider>
achow101: I am mid-way through reviewing that
< achow101>
as that's a requirement for sqlite wallets now
< meshcollider>
But Marco's comment needs addressing anyway
< sipa>
provoostenator: what is debug mode?
< provoostenator>
--enable-debug
< provoostenator>
Which slows rescan down enourmously
< achow101>
I think phantomcircuit had something for rescanning on the block filters
< achow101>
or was working on something for that
< provoostenator>
I think MarcoFalke had a PR, but closed it.
< provoostenator>
Indeed using filters
< provoostenator>
There were some hairy issues around refilling the keypool and updating the filters.
< jonatack>
I plan to get to the #11413 follow-ups soon, e.g. tests, cleanups, and universal explicit feerate argument for RPCs. Needed before feature freeze.
< provoostenator>
One reason I made that PR is that people didn't like overloading arguments like conf_target even more
< jonatack>
I should review 16378 asap actually
< jnewbery>
thanks wumpus!
< achow101>
wen miniscript?
< sipa>
i'll get back to it some time; too many other things to work on right now
< phantomcircuit>
achow101, i have something but kept getting frustrated because the organization of the gcs block filter stuff just seems wrong, it's really hard to extend without h4x
< ariard>
sdaftuar: we may have another slight improvement to eviction logic after #19670, if our slots are full we actually reject inbound connection from past IsDiscouraged peers
< ariard>
but we don't pick them up as prioritized candidates for eviction if they're already connected to us and we get a new inbound
< ariard>
as a first heuristic we could iterate on all IsDiscouraged and randomly pick out for eviction, thus biasing towards not-known-misbehaving peers
< sipa>
ariard: discouraged peers have m_prefer_evict set, which goes into the eviction logic