< gribble>
https://github.com/bitcoin/bitcoin/issues/21236 | Net processing: Extract `addr` send functionality into MaybeSendAddr() by jnewbery · Pull Request #21236 · bitcoin/bitcoin · GitHub
< bitcoin-git>
[bitcoin] jarolrod opened pull request #21357: test: Unconditionally check for fRelay field in test framework (master...test-framework-ver-msg-fix) https://github.com/bitcoin/bitcoin/pull/21357
< bitcoin-git>
bitcoin/master 233a886 Sebastian Falbesoner: test: check that getblockfilter RPC fails without block filter index
< bitcoin-git>
bitcoin/master d099894 MarcoFalke: Merge #20969: test: check that getblockfilter RPC fails without block filt...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #20969: test: check that getblockfilter RPC fails without block filter index (master...2021-test_getblockfilter_with_disabled_blockfilters) https://github.com/bitcoin/bitcoin/pull/20969
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #21358: fuzz: Add missing include (test/util/setup_common.h) (master...2103-fuzzInc) https://github.com/bitcoin/bitcoin/pull/21358
< wumpus>
amiti: will take a look
< vasild>
wumpus: wrt torv2 - if torv2 addresses are non-operational on the tor network, then I think we want to stop trying to connect to such ones and maybe even purge them from addrman.
< bitcoin-git>
bitcoin/master 9d5313d Anthony Towns: move-only: Add txorphanage module
< bitcoin-git>
bitcoin/master 81dd57e Anthony Towns: txorphanage: Pass uint256 by reference instead of value
< bitcoin-git>
bitcoin/master 38a11c3 Anthony Towns: txorphanage: Add lock annotations
< vasild>
jnewbery: ack, 21:00 UTC is usually too late for me, but maybe I will visit the party once in a while :)
< bitcoin-git>
[bitcoin] laanwj merged pull request #21148: Split orphan handling from net_processing into txorphanage (master...202102-txorphanage) https://github.com/bitcoin/bitcoin/pull/21148
< wumpus>
vasild: I'd agree--though the current situation seems more subtle: they still work on the tor network, but future Tor releases will no longer support them, I think with old tor versions they will still work for a while
< vasild>
hmm, right
< wumpus>
vasild: so we might want to make it an option first ! no strong preference though
< vasild>
nothing can prevent old tor nodes from exchaning torv2 between themselves
< wumpus>
if there is wider agreement to nuke v2 support completely for 0.22 that's ok with me
< vasild>
I am not sure either, when is 22.0 freeze?
< vasild>
lets see how connecting to torv2 nodes works in e.g. beginning of June and decide then what to do: 1. do nothing, 2. introduce an option --ignore-torv2 with default of false, 3. --ignore-torv2 with default of true, 4. unconditionally ignore torv2 without any new options
< wumpus>
right, good summary of the options
< vasild>
a test may be something like: run (old) tor node that supports torv2, pick e.g. 100 torv2 addresses from addrman and try to connect each one of them
< aj>
wumpus: i have a draft branch that moves g_cs_orphans into TxOrphanage, it's mentioned in one of the comments
< wumpus>
aj: thanks, good to know (i tried to read the comments but there's so many of them + github likes to play hide and seek with them)
< bitcoin-git>
[bitcoin] t-bast opened pull request #21359: rpc: include_unsafe option for fundrawtransaction (master...fund-raw-transaction-allow-unsafe) https://github.com/bitcoin/bitcoin/pull/21359
< aj>
wumpus: anyway, will open a pr for it sometime soonish (expect the net/chrono and addr-to-net-processing PRs will end up having to come first due to touching related code though)
< aj>
jnewbery: i think aim for merging chrono before addr, yeah?
< jnewbery>
vasild: sorry :( There's no time slot that's nice for everyone and 21:00 UTC seems to minimimze ∑(inconvenience)
< jnewbery>
aj: I don't mind rebasing addr if chrono gets in first, and chrono seems to have a few more engaged reviewers at this point, so yeah lets get that in.
< vasild>
jnewbery: right, can't make it convenient for everybody without a time machine :)
< fanquake>
after a while you get very good at speed reading irc logs
< aj>
MarcoFalke: how frequently does drahtbot update it's conflicts comments?
< bitcoin-git>
bitcoin/master 55e8288 Pieter Wuille: Make all Poisson delays use std::chrono types
< bitcoin-git>
[bitcoin] fanquake merged pull request #21015: Make all of net_processing (and some of net) use std::chrono types (master...pr-20044) https://github.com/bitcoin/bitcoin/pull/21015
< bitcoin-git>
bitcoin/master fa576b4 MarcoFalke: Move MakeNoLogFileContext to common libtest_util, and use it in bench
< bitcoin-git>
bitcoin/master 83bdbbd fanquake: Merge #21003: test: Move MakeNoLogFileContext to libtest_util, and use it ...
< bitcoin-git>
[bitcoin] fanquake merged pull request #21003: test: Move MakeNoLogFileContext to libtest_util, and use it in bench (master...2101-benchNoLog) https://github.com/bitcoin/bitcoin/pull/21003
< wumpus>
does anyone else want to add or remove anything ?
< jonatack>
maybe add #20197 please, if the list isn't too long
< gribble>
https://github.com/bitcoin/bitcoin/issues/20197 | p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage by jonatack · Pull Request #20197 · bitcoin/bitcoin · GitHub
< wumpus>
jonatack: added!
< jonatack>
wumpus: ty!
< wumpus>
is there anything (almost) ready for merge?
< ariard>
hi
< achow101>
#20536 has 3 acks
< gribble>
https://github.com/bitcoin/bitcoin/issues/20536 | wallet: Error with "Transaction too large" if the funded tx will end up being too large after signing by achow101 · Pull Request #20536 · bitcoin/bitcoin · GitHub
< elichai2>
hi
< MarcoFalke>
hi
< ajonas>
There are a few old tests that have got some new review... nothing major though
< wumpus>
achow101: good to know
< ajonas>
#18795, #19393, #14604
< wumpus>
i guess that's all for this topic, let's go to next one then
< jnewbery>
One strange (and potentially footgun-ish aspect) of -blocksonly is that if a transaction is submitted to the node over RPC, or sent by a whitelisted peer, then the node will relay that transaction to all its peers.
< jnewbery>
This makes it very obvious that the node is the origin of the transaction, since it doesn't relay any other transaction!
< jnewbery>
Are there legitimate use cases for this behaviour?
< ariard>
I would say emergency broadcast, you don't care about the privacy
< sipa>
fRelay=false is also set in other cases, right?
< sipa>
for blocks relay connections, even if you're not in blocksonly mode?
< ariard>
like when your main node is upgrading/power outage/...
< jnewbery>
by Bitcoin Core? We set fRelay=false for -blocksonly mode, or for the block-relay-only connections, even if you're not in -blocksonly mode
< sipa>
oh, nvm, i was confusing things; this isn't relevant
< jnewbery>
bloom filter clients also set fRelay=false when connecting to supress tx relay until they've loaded the filter
< sipa>
i'm not sure what the right behavior is
< sipa>
it's clearly a privacy leak, but i think it would also be incorrect to just not relay
< wumpus>
maybe reject it by default but add a 'force' flag to override, i dunno
< sipa>
there is an expectation that a transaction is relayed if you call sendrawtransaction
< fjahr>
I was just going to suggest a force flag too
< wumpus>
yea failing silently would be the worst of both worlds
< ariard>
yeah force flag sounds good
< jonasschnelli>
+1 for the force flag
< jnewbery>
always fail sendrawtransaction if -blocksonly is set?
< jonasschnelli>
jnewbery: seems a bit to hand-holdish
< MarcoFalke>
Seems a bit too hand-holdish to add a force flag for this
< MarcoFalke>
The privacy leak is well documented in the option help
< jnewbery>
a force flag seems the worst of all worlds
< sdaftuar>
can't someone use bitcoin-qt in blocksonly mode? i'm not understanding hte force flag in that scenario
< sdaftuar>
some warnign popup i guess?
< jonasschnelli>
The GUI could have a warning, yes.
< jnewbery>
oh, one thing I should add: -blocksonly will soft-set -walletbroadcast to false
< jonasschnelli>
Which is more or less the UX adaption of a "force" CLI flag
< luke-jr>
MarcoFalke: it fits well with your force-past-policy PR
< wumpus>
it doesn't imply walletbroadca.. right
< wumpus>
so we did think of that
< jonatack>
sendrawtransaction help states the issue clearly
< MarcoFalke>
If we assume that no one reads the docs, what is the point of adding it in the first place.
< jonasschnelli>
yes. that is a valid point.
< jonasschnelli>
Maybe we should expect people read the docs
< jnewbery>
My baseline assumption is that users don't read docs
< amiti>
MarcoFalke: I think its not boolean? I'd expect some users to read docs, but not all
< sipa>
i'm a bit confused about the context... this is only about the sendrawtransaction RPC, not about wallet sending?
< jnewbery>
and I don't think documenting bad behaviour is better than removing bad behaviour
< jonasschnelli>
jnewbery: if so, how do they even find out about the blocksonly mode? :-)
< luke-jr>
jonasschnelli: reading *some* docs but not others! XD
< MarcoFalke>
sipa: I think this is a general discussion about the version message fRelay field
< jnewbery>
jonasschnelli: because someone suggests that they enable it on a twitter thread to save themselves some bandwidth?
< jonasschnelli>
yes... that
< sipa>
what specifically are we discussing changing?
< sdaftuar>
the followup question to this, i believe, is whether it's reasonable for our software to ignore transactions from a peer that has set fRelay=false -- do i have that right jnewbery?
< jnewbery>
sdaftuar: that's the wider context, yes
< sdaftuar>
so this question is to establish whether our current relay behavior is sort of reasonable at all
< jonasschnelli>
Aren't there any valid use-cases that use blocksonly & wallet broadcast (like a private network, designated connection)?
< MarcoFalke>
jnewbery: There is the use case where the node(s) with blocksonly only connect to trusted nodes. It seems too broad to call the feature bad
< jonasschnelli>
Agree with MarcoFalke
< sipa>
MarcoFalke: i'd expect people to set whitelisting in that case, though (or they'll experience more broken behavior)
< sipa>
like rebroadcasting won't work
< jnewbery>
I don't understand this use case
< sdaftuar>
sipa: i think rebroadcasting would work fine? it'll relay to your node in the same circumstances that your node would succeed in relaying it to their peers, no?
< luke-jr>
jnewbery: maybe the user just doesn't care about privacy?
< jnewbery>
luke-jr: that's fair, but is that a use case we want to support?
< luke-jr>
…
< MarcoFalke>
sipa: sendrawtrasnaction is designed to do a rebroadcast every time you call it
< sipa>
MarcoFalke: right, but wallet rebroadcasting won't work, because alreadyhave logic will think the (only) peer already has it
< MarcoFalke>
your inventoryknown is the other peer's alreadyhave ;)
< sipa>
hmm, fair; even if you announce again, the peer won't fetch it again
< sipa>
i'm confused now
< jnewbery>
transaction relay is a public network function, and it should be designed in such a way that is optimal for users connecting to public networks
< sipa>
i thought that was the reason we introduced whitelisting in the first place
< luke-jr>
jnewbery: I never liked the idea of telling users what they can and can't do. And if Core is going to do that, at least the protocol needs to be neutral.
< sdaftuar>
oh! we never clear filterInventoryKnown? that seems potentially problematic
< amiti>
I find it unexpected that the default behavior of -blocksonly mode is to disable wallet broadcast, but if you submit via sendrawtransaction RPC the node will still relay your transaction
< sipa>
doesn't look like it
< sipa>
amiti: that's inconsistent for sure
< sipa>
but i don't really know what to do about it
< jnewbery>
I'm still curious about the supposed use cases. jonasschnelli and MarcoFalke thing there are use cases, but I don't understand them
< ariard>
actually you can be willingly to not participate in tx-relay to mask your connection graph
< ariard>
and only broardcat when you really need it
< amiti>
sipa: what's the issue with just giving the user an error message if they attempt sendrawtransaction?
< amiti>
maybe not desirable, but one way to make it consistent
< jonasschnelli>
jnewbery: Assume someone runs a blockonly node at home (bandwith) but connects it to a trusted peer over wireguard
< MarcoFalke>
sendrawtransaction is explicitly called. Wallet broadcast is implicit, so I think the current behavior makes sense
< MarcoFalke>
(It is possible to enable wallet broadcast explicitly as well)
< amiti>
MarcoFalke: fair
< jnewbery>
jonasschnelli: that'd associate all of your transactions to your node
< luke-jr>
bandwidth vs privacy is a tradeoff. Users may prefer bandwidth.
< sdaftuar>
jnewbery: does that matter to tor users?
< MarcoFalke>
jnewbery: Which is no problem because you control the other node
< lightlike>
Am I right to assume that this issue with sendrawtransaction doesn't apply to regular blocks-only connections in normal mode - only in full blocksonly mode?
< ariard>
but even if you don't control the other node, you prefer the transaction propagating over the privacy gain
< jnewbery>
MarcoFalke. Presumably the home node is relaying the transaction on to the network?
< * luke-jr>
wonders if Tor nodes can/should make a dedicated connection just for broadcast
< amiti>
lightlike: yes
< ariard>
you *might* prefer (e.g time-sensitive txn)
< MarcoFalke>
sdaftuar: If you don't control the tor node, I'd say yes. (The other peer will learn your whole wallet contents over time)
< sipa>
so sendrawtransaction does not relay over fRelay=false connections in general, only in -blocksonly mode?
< jnewbery>
sipa: I belive that is correct
< sipa>
curious
< MarcoFalke>
sendrawtransaction will send it out to all peers that have send you a fRelay=true
< sdaftuar>
fRelay=false isn't a "category" of connection, really
< luke-jr>
jnewbery: in that scenario, the home node talks exclusively to your dedicated server somewhere over VPN
< MarcoFalke>
sendrawtransaction doesn't take into account what fRelay you sent to the peer
< sdaftuar>
we have full-relay-outbound connections, which might actually be fRelay=false if we're in -blocksonly mode
< jnewbery>
luke-jr: I don't think that's what jonasschnelli was describing
< sipa>
ah, i see
< sdaftuar>
but we treat them as our tx relay peers anyway (which is correct, in the sense that fRelay=false only instructs our peer to not relay to us, not the other way around)
< sipa>
jnewbery: that's how i interpret jonasschnelli's scenario as well
< jonasschnelli>
yes. m2
< sipa>
(only one connection, blocksonly to a trusted peer; you don't care about privacy w.r.t. that peer)
< sipa>
i think in general "i don't care about privacy w.r.t. this peer" is something to be captured by whitelisting
< sipa>
but of course... who knows what people read
< luke-jr>
hmm
< MarcoFalke>
Another use case might be to create a larger gap from your hot wallet to the live p2p network (multiprocess isn't a thing yet)
< ariard>
sipa: whitelisting is static, you might willingly to bypass "i don't care about privacy w.r.t this peer" for a one-shot broadcast
< MarcoFalke>
sipa: It is called permission flags now ;)
< sipa>
MarcoFalke: right, thanks
< jnewbery>
ah, thank you. I was misunderstanding. I think if you control both ends of the connection, then it'd make more sense for the home node to set feefilter=max to the trusted peer
< sipa>
ariard: that seems more like something you'd want for tor connections (and as a specific way of broadcasting transactions)
< MarcoFalke>
And permission flags don't work for outbound connections (yet)
< luke-jr>
MarcoFalke: have a PR for that for a year+ now :P
< jonasschnelli>
Just saying because if we flag blockonly privacy leaking we imply normal operation isn't
< MarcoFalke>
sdaftuar: We also assume the node to support BIP 60 for blocksonly, so it is not a large change?
< sipa>
if we believe that wallet privacy is a lost cause, there is no reason to improve it; but i don't think it is, and with improvements in the pipeline i don't think suboptimal privacy right now is a reason not to think about other issues
< MarcoFalke>
(nodes will be disconnected if they don't support BIP 60, no?)
< sdaftuar>
MarcoFalke: well my guess is that bip37/bip60 is more widely supported on the network. feefilter has always been optional
< jnewbery>
MarcoFalke: BIP60 is version 70002. feefilter is version 70013.
< amiti>
sipa: strong +1
< jonasschnelli>
yes. thats a good point sipa
< jnewbery>
but I think being able to send feefilter before verack would be nice
< sipa>
still, i'm not sure exactly what we're talking about? blocksonly + sendrawtransaction? or also blocksonly + walletbroadcast + wallet send? something else?
< sdaftuar>
but wait feefilter doesn't help right jnewbery?
< jnewbery>
feefilter doesn't help here, no
< jnewbery>
we'd still need BIP 338
< wumpus>
both security and privacy are always about making attacks marginally more difficult/expensive, not absolutes, would not call it a lost cause
< jnewbery>
some way to signal tx relay
< sipa>
wumpus: right, my point was that sometimes an argument of "you're trying to improve X, but X is always going to be bad because of Y, and we can't fix Y" is a valid argument not to work on something... but i don't think that's the case here
< jnewbery>
sipa: the specific reason I'm asking is that other than these edge cases, fRelay=false means "I don't want txs on this connection and I'll never send txs on this connection". That's a nice property
< sipa>
what happens right now if someone runs bitcoin-qt in -blocksonly mode and creates a transaction?
< sdaftuar>
that's only true in a world where NODE_BLOOM isn't supported
< jnewbery>
I was trying to understand if those edge cases are actually important, or whether they're just things that people are speculating about
< jonatack>
jnewbery: only the first of the two?
< jnewbery>
*only true for nodes that don't support serving bloom filters, yes
< luke-jr>
jnewbery: didn't sdaftuar just add a new flag for that?
< sipa>
while still supporting fRelay
< wumpus>
sipa: sure, agree
< sdaftuar>
luke-jr: not merged yet
< jnewbery>
sipa: it wouldn't be broadcast because walletbroadcast is set to false. I don't know what the GUI shows
< sipa>
that seems bad
< luke-jr>
sdaftuar: right, but it would make fRelay having this property redundant..?
< * sipa>
conjectures that barely anyone uses -blocksonly on nodes they actually use as wallets
< jnewbery>
this was also my conjecture
< sdaftuar>
luke-jr: i think jnewbery's proposal is to scrap the BIP
< luke-jr>
ah
< wumpus>
i don't think blocksonly was ever officially supported for the GUI, it works, sure
< MarcoFalke>
so replace BIP338 with a breaking p2p change and blocksonly meaning "feefilter=max"?
< jnewbery>
my original proposal was to script fRelay, but it seems that people rely on it to mean 'blocks only'
< wumpus>
but it was originally intended to be some kind of internal, rarely used thing
< wumpus>
not something GUI end users would face
< jnewbery>
*scrap fRelay
< ariard>
fwiw, core doesn't even commit to bip60 as a supported bip
< MarcoFalke>
ariard: It only does when -blocksonly
< darosior>
Fwiw i believe the GUI would show a fee estimation error in this case if no fallbackfee is set
< wumpus>
e.g. there is no configuration dialog option to enable it
< jonasschnelli>
darosior: Yes. It will. But users with just set a manual fee
< sipa>
or at least, we don't assume our peers do
< luke-jr>
sipa: we send it
< luke-jr>
iirc
< jnewbery>
we always send fRelay, but don't expect our peers to
< sipa>
luke-jr: BIP60 is about predictably-sized version messages, not just the fRelay flag
< jnewbery>
we also don't require that our peers send starting height, subversion, etc
< luke-jr>
sipa: but on the sender side
< sdaftuar>
sipa: but we're compatible with bip60 peers, and i think we now expect our peers to always read our fRelay flag if its' set to false, right?
< ariard>
even the original bip37 is really unclear if fRelay semantic is to disable tx-relay or just tx-announcement
< luke-jr>
it doesn't require breakign non-BIP60 peers
< sipa>
sdaftuar: hmm, true
< jnewbery>
sdaftuar: you mean we expect them to read fRelay because we'll disconnect them if they send us a tx?
< sdaftuar>
right, if we set fRelay=false i think we now disconnect if they ignore it
< ariard>
unless you signal NODE_BLOOM
< MarcoFalke>
Bitcoin Core implements BIP60, but doesn't require peers to implement it. (Like any other optional BIP)
< jnewbery>
I'm not sure we do disconnect peers for that
< amiti>
orly? I thought we disconnected for that too
< sdaftuar>
jnewbery: it is possible i'm misremembering , i know i thought this at some point in the past and was wrong
< sipa>
so i guess we could list BIP60 as implemented, actually
< MarcoFalke>
jnewbery: I am pretty sure there is a "tx sent in violation of protocol" log
< ariard>
iirc #15759 introduce the change of disconnecting block-relay-only peers sending us a transaction
< MarcoFalke>
ajonas: Thanks for summing those up!
< jnewbery>
MarcoFalke: yup. You're right
< wumpus>
ajonas: thanks for doing this
< ajonas>
Happy to!
< jnewbery>
thank you ajonas!
< MarcoFalke>
One major theme is "It is unclear which status a pr is in". I was thinking how to improve that, but couldn't find a solution
< ajonas>
I think a labeling system might be worth trialing
< MarcoFalke>
Obviously it can't be an objective metric (that a computer could measure), because then it would be gameable
< amiti>
imo, communicating PR status should be the responsibility of the pr author instead of maintainers
< ajonas>
But we could also add some meta data in the first comment that can be updated over time
< wumpus>
what are the PR statuses in the first place?
< MarcoFalke>
I think the best metric is a heuristic "A pr is closer to merge the more ACKs it has relative to its potential risk"
< wumpus>
i mean we have a few like 'waiting for author' 'needs rebase' but not sure what you mean here
< ajonas>
I think concept / approach ack vs deep code review
< wumpus>
MarcoFalke: yes, definitely
< MarcoFalke>
wumpus: I'd say when there is no "needs rebase" label, the status is implicitly "needs review"
< wumpus>
ok so like 'needs concept ACK'
< wumpus>
we could add that, but yes, like amiti says, authors have to request this
< ariard>
draft status doesn't mean "needs concept ACK"?
< MarcoFalke>
I am not sure if that makes sense. How would you know when to remove the "needs concept ACK" label?
< wumpus>
ariard: maybe
< ajonas>
I think the use of draft is inconsistent
< wumpus>
ariard: we could use it for that
< wumpus>
MarcoFalke: good point
< ariard>
good thing with draft you can do it just a pr author, doesn't special repo permisions iirc
< wumpus>
yes that is good
< wumpus>
self-organizaing is good, anything that gives the maintainers more on their plate is probably not going to fly
< amiti>
something I'm trying on #21061 is adding an explicit "status" section at the top of OP. not super relevant yet because the PR hasn't gotten much attention, but was hoping to make this easier for reviewers as the comments accumulate
< amiti>
wumpus: yeah, its a bit weird because at the end I'd have to remove status when I thought its RFM? or potentially have the status included in the merge commit?
< sdaftuar>
amiti: maybe try to make it the second post instead? a bit annoying but would solve that issue, too late now though i guess!
< MarcoFalke>
sdaftuar: I think the point is that a miner can't mine a block during IBD? getblocktemplate will fail
< sdaftuar>
MarcoFalke: -maxtipage exists for that exact reason
< MarcoFalke>
ah
< amiti>
sdaftuar: haha, not too late.. I also have description in second post (additional info I thought useful for review, but unnecessary for merge commit) so I could edit, but I think its more useful if its very apparent when someone comes to the page
< jonatack>
Out of curiosity, has a weekly or fortnightly random review distribution bingo ever been tried? e.g. people sign up to receive a randomly chosen PR to review. It might get people looking at new code or reviewing new people.
< sipa>
jonatack: heh
< sdaftuar>
amiti: ah, nice! btw i did appreciate the extensive background in that PR, will be helpful when i get to it, thanks for doing that!
< sipa>
jonatack: i might sign up
< amiti>
sdaftuar: :)
< jonatack>
sipa: same
< jonatack>
ajonas: wdyt
< aj>
wow, that was a lot of frelay discussion
< sipa>
morning, aj
< phantomcircuit>
sdaftuar, no i dont think that's right, we don't even inv if we support compact blocks do we?
< aj>
ajonas: your word cloud is almost subliminal messages: "core people think feel time" (we're high?) "code review progress consensus just maybe" (so positive) "PRs work, yes? help contributors process taproot wallet" (bribery?)
< sipa>
needs haiku
< luke-jr>
wut
< aj>
phantomcircuit: isn't that only for the couple of peers we select/who select us for fast compact block relay?
< sdaftuar>
phantomcircuit: i believe that as long as we're out of IBD, we'll always announce a new block via inv, header, or cmpctblock
< sdaftuar>
a header or compactblock will only be sent if we think it will connect to the peer's headers chain, based on prior headers sent or received
< sdaftuar>
(along with other requirements)
< sdaftuar>
so the point is, if a block is found, it should get announced
< sdaftuar>
and if the header doesn't connect, or if we get an inv, i believe we always send a getheaders to the peer that gave us the block
< sdaftuar>
once we have the header chain, we can sometimes direct fetch the blocks (when the header is received), and sometimes we rely on the parallel download logic. i think the latter is the issue, as we don't call it on inbound peers while in IBD, if we have outbound peers
< sdaftuar>
and if we're in IBD the direct fetch is disabled
< aj>
jnewbery: fwiw, i had the idea at one point that -blocksonly could relay dandelion++ stems (of txs that don't depend on in-mempool parents) and get some privacy that way, obv pretty long-term though
< phantomcircuit>
sdaftuar, on receipt of a compact block we only do getheaders if we're not in IBD
< sdaftuar>
don't we send a getheaders if it doesnt' connect to our tip?
< sdaftuar>
also - we wouldn't get a compact block if we're in IBD because we wouldnt' have told anyone to send us unrequested compact blocks
< sdaftuar>
oh- i see what you're saying, we seem to ignore compact blocks that don't connect if we're in IBD. that is indeed odd.
< sdaftuar>
however i don't think that can be implicated here, as it should never occur
< sdaftuar>
(i also think we should fix that so we treat it the same as an unconnecting header, always)
< phantomcircuit>
sdaftuar, only if we're not in ibd
< andytoshi>
achow101: in BIP174, the test vector "PSBT with unknown types in the inputs" describes an input which has keytype 0x0f and key 0102030405060708. this is a valid PSBT
< andytoshi>
but in PSBT2 it's invalid because keytype 0x0f is now the sequence number
< andytoshi>
is it worth updating the text of PSBT0 for this
< achow101>
andytoshi: probably. I think I updated it in the implementation pr but not the bip pr
< andytoshi>
oh ok i'll see if i can find that .. you're saying there's a replacement test vector in the implementation?
< achow101>
andytoshi: I've pushed the updated tests to my bips pr too
< andytoshi>
oh thanks!
< phantomcircuit>
sdaftuar, why would that never occur?
< sipa>
achow101: do we ever set the requested hashtype flag in PSBTs we create?
< sipa>
(in bitcoin core)
< achow101>
sipa: no
< achow101>
err, I don't think so
< sipa>
to deal with taproot's SIGHASH_DEFAULT (0), i'm thinking of just making SIGHASH_DEFAULT the default everywhere, but in the non-taproot signing function treat SIGHASH_DEFAULT as SIGHASH_ALL
< sipa>
but if we ever leak a hashtype via PSBT that might result in emitting a 0 there, which other signers would consider nonsensical
< achow101>
seems reasonable
< sipa>
also i think in some places we treat sighash=0 as "unspecified"
< achow101>
yes
< achow101>
I think we would only output a sighash type in a psbt if it was specified in a psbt
< achow101>
and iirc none of the rpcs take a sighash type
< sipa>
achow101: any reason why this wouldn't work? you create two descriptor wallets, one with keys disabled, and the other doesn't, you import an xpub descriptor in one, and xprv in the other (one for internal and one for real)... you run walletcreatefundedpsbt on one, walletprocesspsbt on the other
< sipa>
i try this, and it correctly signs the first transactions, but not any subsequent ones
< achow101>
change?
< achow101>
it should work, and I think there's tests for it
< sipa>
neither transaction has change inputs, but i do have an internal descriptor imported in both
< bitcoin-git>
[bitcoin] sipa opened pull request #21365: Basic Taproot signing support for descriptor wallets (master...202102_taproot_sign) https://github.com/bitcoin/bitcoin/pull/21365
< achow101>
ooh taproot
< real_or_random>
TIL when you click on the file name at the top of the "code comment box" in a review on github, you get the exact commit range the reviewer was looking at when writing the comment