Properly document target_confirmations in listsinceblock

[RHavar]

There seems to be some misunderstandings about this, but it's a heavily used function so I'd like to make sure the docs are clear about how it works.

For a later issue:

• Change the default of target_confirmations to 6 (1 is a pretty silly default)
• Change the name of target_confirmations (it's really a horrible name)

[TheBlueMatt]

utACK I know this is a super old issue but I find the current docs astoundingly confusing, can we try for 15?

[instagibbs]

@kallewoof I think you have experience with this call?

documentation sounds reasonable, but I don't actually know how it's used.

[RHavar]

The way the API is designed to be used is like this:

Let's say I care about only rescanning of 10:

let confsCareAbout = 10
let  hash = ""
while (true) {
     let res = exec(`bitcoin-cli listsinceblock ${hash} ${confsCareAbout)`)
     process(res.transactions)
     hash = res.lastblock;
}

It's a pretty sweet work flow for handling deposits. Just unfortunately listsinceblock is too buggy and ill-defined (I'll create some bug reports when I can).

But this PR fixes the docs, that makes it a bit clearer what it does. Most people when they read the docs think it's a filter, when it's not.

[TheBlueMatt]

Maybe add a note that transactions with fewer confirmations than this are still included in the response, only lastblock is changed by this argument.

[morcos]

NACK as is

The documentation is very misleading. But it needs to be made even more clear than this.
Perhaps add "Transactions with any number of confirmations in the range from blockhash to current tip (or the fork point if blockhash is not on current chain) will be returned." somewhere

[RHavar]

(or the fork point if blockhash is not on current chain)

Is that what it actually does? o.0 If so thats ....bizarre and wtf.

[TheBlueMatt]

Yes, thats what it appears to have always done...docs need to be a shitload clearer here.

[RHavar]

I pushed another stab at documenting it based on what @morcos said, but it honestly makes no sense to me. I assume I'm misunderstanding.

Let's imagine I have the blocks:

           /--> C 
A--->B
           \-->D -->E

Where E is the main chain tip. And C is orphaned.

So you're saying if I call listsinceblock C, it's going to return me only transactions between [B, C) AKA transactions I've already processed, and are orphaned?). The expected set of transactions it would return you is between [B, E).

[morcos]

Yes it will return between B and E

[RHavar]

Hm, so when you said:

Transactions with any number of confirmations in the range from blockhash to current tip (or the fork point if blockhash is not on current chain) will be returned.

You mean that just the lastblock returns $target_confirmations from the (mainTip or convergencePoint) ?.

But this is independent to the transaction filtering stuff?

[TheBlueMatt]

No, the value returned is always the Nth-from-top block on the best chain.

[RHavar]

Yeah, that's what i thought ><

[kallewoof]

target_confirmations seems to be handled in a very weird way right now. It only affects the returned last block, and does not actually affect the transactions being returned in any way. I would declare this a bug, personally. The only time the value is used is at https://github.com/bitcoin/bitcoin/blob/0d457c7b274a66d66d59725f5fe1ee1cef5b6391/src/wallet/rpcwallet.cpp#L1859 where

    CBlockIndex *pblockLast = chainActive[chainActive.Height() + 1 - target_confirms];

i.e. to determine the block hash of he last block.

Correct me if I'm wrong.

[TheBlueMatt]

@kallewoof you are correct, but that behavior long predates the documentation for the function. I have no idea if anyone uses its previous behavior, but assuming people test things before putting them in production, best to fix the docs and leave the behavior IMO.

[kallewoof]

@TheBlueMatt That makes sense to me.

[RHavar]

I don't think the behavior is a bug at all, it was intentionally designed for use in a loop like I describe in an earlier comment.

It's actually a really nice and simple way to handle deposits, and I know a few people who do it that way. Note how it works really well if your depositor goes down, because you keep storing the hash you want to scan from. Changing the behavior at this point would be totally unreasonable.

(The only reason I don't handle deposits myself that way, is how it handles double spend (attempts) are weird and inconsistent, which make it very difficult to work with when you're showing unconfirmed stuff)

[morcos]

ACK 52295ba

I think that language is clear and accurate

(except I'm not sure about putting brackets around the argument names in the help text, is that something we do anywhere else?)

[RHavar]

(except I'm not sure about putting brackets around the argument names in the help text, is that something we do anywhere else?)

I just copied it from the original. It's a bit messy, but it's pretty clear and makes things a lot less ambiguous.

[kallewoof]

(The only reason I don't handle deposits myself that way, is how it handles double spend (attempts) are weird and inconsistent, which make it very difficult to work with when you're showing unconfirmed stuff)

The reason why double spends are weird and inconsistent is probably because you are using target_confs > 1. This means if there's a 1-2 block fork, and your target_confs = 10 or something, you will not be told of the differences caused by the orphan since you're listing a block that preceded the fork point.

That's how I interpret this, anyway.

[kallewoof]

My initial expectation was that transactions with less than target_confs confirmations (i.e. transactions in blocks newer than the returned lastblock at the end) would be excluded from the results. This would give you a buffer of 0-confs and 1-confs that are still at risk. That's not what is happening though.

[TheBlueMatt]

"rescanning" means something specific, maybe "so you will be continually re-notified of transactions until they've reached 6 confirmations"

[laanwj]

Needs rebase and addressing of @TheBlueMatt 's nit, if this is still to make 0.15

[RHavar]

Ok, rebased and addressed Matt's nit

[TheBlueMatt]

Looks like a few rebase errors.

[TheBlueMatt]

Rebase error here, too.

[TheBlueMatt]

Looks like a rebase error - you've removed the include_removed parameter here.

[RHavar]

Sorry, yeah. I fail. Pushed a fix.

[TheBlueMatt]

Why did you delete the \n here?

[RHavar]

some how screwed that up in the rebase. fixed

[RHavar]

Ok, fixed my rebase fail...

[TheBlueMatt]

utACK.

[TheBlueMatt]

I dont think "bestblockhash" is one word.

[RHavar]

Ok. 😭

[TheBlueMatt]

utACK

[laanwj]

utACK 9f8a46f