< morcos>
i'm not sure how we made that mistake in the first place, we explicitly discussed this problem in IRC on 5/5, but in any case, i'm not loving your solution.
< sipa>
morcos: oh, what's the problem with it? (i can't remember the discussion about it)
< morcos>
i think there is too much complication around settingbestchain that caused this to happen in the first place. setbestchain is not expensive, lets just call it whenever we flush the utxo set, then we're only worried about keepign one thing in sync
< morcos>
but also i don't like moving the unlinking of files down to where you moved it
< morcos>
once you modify the blockindex database, they are useless anyway
< sipa>
hmm?
< morcos>
and its slightly beneficial to delete them before the CheckDiskSpace call
< sipa>
right, i started writing this under the assumption that UnlinkPrunedFiles modified the block index
< sipa>
i feel there is a cyclic dependency here
< sipa>
the wallet shouldn't be updated until the chainstate is updated
< sipa>
the chainstate needs to have the block index flushed first
< sipa>
pruning deleted files, so should be done after updating any reference to them
< morcos>
we already "solved" the problem of keeping the utxo state in sync with pruning files
< morcos>
lets reuse that solution
< morcos>
just move setbestchain inside the block above that's if fDoFullFlush
< morcos>
the utxoset and the wallet will then be permanently in sync
< sipa>
well it means that the wallet can be ahead of the chainstate
< sipa>
but that shouldn't be a problem, i guess?
< morcos>
wait i'm confused by the terminology one of us is using
< sipa>
you're suggesting to put the "GetMainSignals().SetBestChain(chainActive.GetLocator());" above the "if (!pcoinsTip->Flush())" line?
< morcos>
or right after, but in that code block
< sipa>
if you put it after, the wallet can be behind the chainActive, and you get the pruned beyond error
< sipa>
but the wallet is allowed to be ahead of the chainstate
< morcos>
sipa: no, i don't think so
< morcos>
if what you were saying was true, then it would be possible for your chainstate to be behind your pruned blocks as well
< morcos>
which hopeuflly isn't
< morcos>
possible
< sipa>
if you first write X, and then write Y, you only have a guarantee that the state on disk for X is >= that of Y
< morcos>
ok so lets back up one second, what stops the problem of pruning and then not yet having written the chainstate , and crashing before you do
< sipa>
nothing
< sipa>
i think the correct thing to do is to first do the blockchain AND chainstate write without considering pruning, then doing findfiles to prune, then potentially writing the blockindex again, and then deleting files
< morcos>
yeah, perhaps, i think we didn't do it that way because of wanting to free up space on disk with the pruning to make room for writing the chainstate
< sipa>
or we could remember the last written chainstate sync, and never prune beyond that
< sipa>
and then loop twice
< sipa>
also, the wallet beyond chainstate check will fail in case of a reorg...
< sipa>
it checks whether the wallet state is a descendant of the chainstate tip; that's too strong
< sdaftuar>
sipa: where is that wallet check? i thought we call FindForkInGlobalIndex on the wallet locator versus chainActive.Tip
< sdaftuar>
and rescan from there
< morcos>
OK, well to get back to a short term fix, I'd recommend not moving the UnlinkPrunedFiles call and changing the additional condition in the existing if block to be fDoFullFlush instead of fFlushForPrune
< sipa>
sdaftuar: oh, yes!
< morcos>
Then we can fix it more completely later, but we'll at least have reduced the problem to catastrophic crashes in the middle of FlushStateToDisk
< sipa>
morcos: ack
< sipa>
morcos: actually, i think that flushing the wallet may be more expensive than chainstate flushes in some cases
< morcos>
yeah thats what i was just getting confused about
< morcos>
setbestchain only writes the locator
< sdaftuar>
we're just writing one locator to the db, right?
< morcos>
how does the rest of the wallet keep in sync with that
< sipa>
the wallet is regularly checkpointed
< sipa>
and any change to the wallet is written immediately
< sipa>
but that's not a problem, because the wallet is allowed to be ahead of the chainstate
< sipa>
and it used to also be allowed to be behind... until pruning
< morcos>
yeah sorry, i'm not familiar with the wallet database, but it seems to me that there are lots of writes to the wallet that aren't immediately written, what causes those to be flushed?
< sipa>
there is a thread that occasionally flushes
< morcos>
so what stops setbestchain from being ahead of that?
< sipa>
nothing, i guess...
< wumpus>
actually there are a lot of flushes to the wallet
< wumpus>
what the 'flush thread' does is not actually flush, but make wallet.dat self-contained
< sipa>
ah
< sdaftuar>
wumpus: can you explain? i am trying to read/understand that now
< morcos>
sdaftuar: its in the documentation
< sipa>
sdaftuar: writes to the wallet are synchronous i think
< morcos>
:)
< sipa>
but only to the db logfile; not to wallet.dat
< sdaftuar>
ah ok
< wumpus>
yes it's best to check the berkeleydb documentation, but AFAIK we do a flush after almost every operation
< wumpus>
this is what made some things so slow
< sdaftuar>
there are some operations that are commented as not doing a flush, for performance reasons
< sipa>
morcos: what about moving the wallet update to be above the chainstate write (and always triggering it in case of a pruning)?
< wumpus>
this has been improved a bit for some commands in 0.12, but it's still erring on the safe side not the unsafe one
< sipa>
morcos: that way the wallet will always be ahead of the chainstate when pruning
< wumpus>
sdaftuar: as I understand it, every creation of a CWalletDB(strWalletFile) makes an operation, that will be flushed when the object is destroyed
< morcos>
wumpus: // Do not flush the wallet here for performance reasons // this is safe, as in case of a crash, we rescan the necessary blocks on startup through our SetBestChain-mechanism
< morcos>
is a common comment
< sdaftuar>
wumpus: there's an optional 3rd argument to the CWalletDB constructor, to avoid flushing on close
< wumpus>
sure, there are some places where flushing is skipped, just trying to dispel the myth that the only flushing happens in the flush thread :)
< sdaftuar>
ah ok
< wumpus>
sdaftuar: yes
< wumpus>
it was added relatively recently
< sipa>
wumpus: no :)
< sipa>
i think that trick existed in the 0.3.x codebase
< wumpus>
ok
< sipa>
we should do that during keypool topup, btw
< sipa>
which is ridiculously slow
< morcos>
wumpus: but what i'm trying to understand is how in those places where it isn't "flushed" is it supposed to be depending on SetBestChain to flush it?
< wumpus>
keypool topup has already been sped up in 0.12 afaik
< sipa>
morcos: the flush isn't prevented; just delayed
< sipa>
morcos: it flushes when that CWalletDB object goes out of scope
< wumpus>
morcos: on the next operation, or shutdown, whatever happens first
< wumpus>
sipa: he means when the no-flush argument is set
< sipa>
wumpus: there is no no-flush argument
< sipa>
just a second CWalletDB object that lives longer; the flush only happens when there are no CWalletDB objects
< morcos>
heh, i was about to offer to shut up if it was just me beign confused
< wumpus>
sipa: that's not news :)
< phantomcircuit>
<morcos> if what you were saying was true, then it would be possible for your chainstate to be behind your pruned blocks as well
< phantomcircuit>
morcos, that actually happens and causes a crash on restart
< sipa>
wumpus: the long-living CWalletDb object exists to prevent the _checkpointing_ thread (caled ThreadFlushWalletDb)
< wumpus>
that no one really understands the wallet code is a common argument why people don't like it
< sipa>
wumpus: the fFlushOnClose argument is to prevent flushing to the logfile
< wumpus>
sipa: right!
< wumpus>
ThreadFlushWalletDb has a stupid name
< wumpus>
ThreadCheckpointWalletDb would be much better
< wumpus>
everyone gets confused on that
< phantomcircuit>
wumpus, in practice virtually all operations actually compact the wallet since that background thread triggers within a few seconds of any operation
< wumpus>
I probably only learned it about a year ago too
< morcos>
you are using "checkpoint" to mean moving from logfiles to wallet.dat?
< sipa>
morcos: yes, BDB terminology :)
< sipa>
actually, i'm not sure
< morcos>
but you are saying that all wallet writes are synchronously flushed to log files?
< phantomcircuit>
sipa, same term is used in all sane sql databases
< wumpus>
morcos: yes that is the assumption
< phantomcircuit>
morcos, that they are, it's why the thing is so slow
< sipa>
morcos: flushing to logfiles is prevented with the CWalletDb fFlushOnClose bool
< wumpus>
morcos: if you can disprove it, we found a huge issue
< sipa>
morcos: flushing to wallet.dat is prevented by having a concurrent long-lived CWalletDb object
< sipa>
so SetBestChain synchronously writes to the logfile
< phantomcircuit>
wumpus, trust me virtually all wallet functions cause *multiple* fsync calls
< sipa>
phantomcircuit: yes, writing to the log file causes an fsync
< morcos>
so can i walk through an example
< morcos>
new block comes in
< morcos>
you call SyncTransaction for every tx in block
< morcos>
this walletdb is opened with fFlushOnClose = false
< morcos>
then you call SetBestChain because its time to
< phantomcircuit>
sipa, hmm actually the only places we dont flush might cause a problem
< morcos>
does the call to SetBestChain, which just contains a write
< phantomcircuit>
AddToWalletIfInvolvingMe doesn't cause a flush
< sipa>
morcos: that will flush all writes, including the new txn
< morcos>
somehow cause the AddToWalletInvolvingMe calls (inside SyncTransaction)
< morcos>
ok they get flushed to
< sdaftuar>
so that could be a slow operation
< morcos>
so thats why it can be expensive is what you're syaing
< phantomcircuit>
// this is safe, as in case of a crash, we rescan the necessary blocks on startup through our SetBestChain-mechanism
< sipa>
morcos: yeah, there is a single Db* object which is cached, and every CWalletDb updates gets redirected to that
< phantomcircuit>
is that correct with pruning?
< sipa>
phantomcircuit: all pruning requires is that the wallet is not behind the chainstate
< sipa>
so i think we should first write the wallet.setchainstate, and then flush the chainstate
< phantomcircuit>
well i think in practice there's little risk of that because SetBestChain causes a wallet flush
< morcos>
sipa: that sounds good to me, i doubt the order really matters b/c it seems like we have problems anyway if we crash inside FSTD but can't hurt. But i'd make the additional condition fDoFullFlush and not fFlushForPrune if it were me...
< morcos>
if nothing else, will help the wallet stay more close to caught up during a reindex
< sipa>
phantomcircuit: in the current code, if you crash in between the chainstate update and the wallet update, you can see a wallet that refers to a pruned block
< phantomcircuit>
sipa, and actually CDB::Flush does a checkpoint also
< phantomcircuit>
txn_checkpoint
< sipa>
phantomcircuit: that's a checkpoint to the logfile
< phantomcircuit>
sipa, it also flushes the log file
< morcos>
sipa: in i think a related question, did you see what i asked on #7491. why do we even need to be calling MarkConflicted on loading the wallet?
< phantomcircuit>
the bsb logic around this is all insanely confusing
< sipa>
morcos: backward compatibility
< morcos>
?
< sipa>
i think (i haven't looked)
< sipa>
when you load a wallet that was written by a pre-0.12, it won't have conflict information
< phantomcircuit>
for example DB->sync doesn't always cause an fsync call
< phantomcircuit>
>.>
< sipa>
phantomcircuit: bsb, that's BDB on LSD?
< phantomcircuit>
er
< morcos>
hmm... but aren't you only creating a weird subset of the conflict information
< phantomcircuit>
they're like, right next to each other mannn
< sipa>
morcos: yeah, it can't be complete unless you rescan iirc
< morcos>
ok
< sipa>
which is in the release notes, i think!
< sdaftuar>
so one question about the wallet being ahead of the chainstate: should we be checking that conflicted block information is for blocks that are on chainActive at startup?
< sdaftuar>
not sure i have a sepcific broken scenario in mind
< sipa>
sdaftuar: that's checked at runtime
< sdaftuar>
ah okay
< sipa>
if the conflicted block hash refers to a non-active block, it's considered to be non-conflicted
< phantomcircuit>
i've got a question
< phantomcircuit>
does anybody actually use the conflict logic?
< morcos>
phantomcircuit: it was originally created to distinguish between txs that just aren't in your mempool and txs that are actually conflicted
< morcos>
so that you wouldn't be able to auto doublespend the first
< sipa>
morcos: so i think moving the wallet flush doesn't help, unless we also move the chainstate write to be ahead of pruning
< sipa>
just going to change the wallet write condition
< morcos>
sipa: agreed, and unmove the Unlink?
< sipa>
morcos: yes
< morcos>
sipa: Should I change CheckSequenceLocks to take a CoinsViewCache instead of create its own so that it doesn't need to be inside the mempool lock in ATMP?
< morcos>
or not bother
< sdaftuar>
looks like travis isn't running #7502 for some reason?
< sdaftuar>
never mind - it did
< phantomcircuit>
sdaftuar, there's a limited number of concurrent runs available
< phantomcircuit>
with heavy activity it can take a long time
< sdaftuar>
oh does it take a while for it to even get queued?
< sdaftuar>
looks like sipa's first commit in #7502 was run, but when he force pushed a new commit, that one hasn't been run, and doesn't seem to be queued that i can see
< GitHub186>
bitcoin/master 40e7b61 Wladimir J. van der Laan: wallet: Ignore MarkConflict if block hash is not known...
< GitHub186>
bitcoin/master c9da9c4 Wladimir J. van der Laan: Merge #7491: wallet: Ignore MarkConflict if block hash is not known...
< GitHub29>
[bitcoin] laanwj closed pull request #7491: wallet: Ignore MarkConflict if block hash is not known (master...2016_02_wallet_markconflict_assert) https://github.com/bitcoin/bitcoin/pull/7491
< morcos>
sipa: weird. the unit test passes for me but i don't understand how. in any case, should i just LOCK(mempool.cs) in CheckSequenceLocks or leave it as AssertLockHeld and lock it in miner_tests
< morcos>
the miner test do all kinds of messing with the mempool anyway, so wouldn't be weird to lock it for the whole test
< morcos>
but just want to think about moving these locks in the right direction
< morcos>
its such a mess now without how much cs_main is used to protect mempool stuff already
< morcos>
s/without/with/
< sipa>
morcos: i generally consider functions that optionally run in a locked context bad design
< sipa>
so i'd say leave the assert in CheckSequenceLocks, and add a LOCK in the tests
< morcos>
any idea why the unit test passes for sdaftuar and i?
< sipa>
do you run with -DDEBUG_LOCKORDER ?
< sipa>
(no idea if that's done by default or only specifically from travis)
< morcos>
no.. so AssertLockHeld only applies with DEBUG_LOCKORDER?
< morcos>
i never knew that, i thought it was just an assert
< GitHub22>
bitcoin/0.12 00ec73e Wladimir J. van der Laan: wallet: Ignore MarkConflict if block hash is not known...
< GitHub22>
bitcoin/0.12 1329963 Pieter Wuille: Update the wallet best block marker when pruning...
< morcos>
sipa: i'm not sure i follow exactly what you're saying about optionally running in a locked context. you don't like it when the locks take advantage of being reentrant?
< sipa>
morcos: indeed
< sipa>
morcos: locks should be internal to some level of encapsulation, and not escape it
< morcos>
the downside to locking in the tests, is it'll work fine now (i think) but what happens latter if CreateNewBlock spins off new threads to do things and can't lock the mempool if we've got it locked for the whole test
< morcos>
that would mean we'd need to separately lock for each call to CheckSequenceLocks in the test
< sipa>
morcos: if necessary, have two versions of a function; an internal one that asserts the lock and is not public, and an external one that just locks and grabs the internal one. non-reentrant locks are also signifciantly more efficient
< sipa>
morcos: that's theory, of course...
< sipa>
morcos: meh, just lock once; fixing tests can be done later
< sipa>
morcos: see addrman.h :)
< morcos>
ha! thats how we got into such a messy locking situation in the first place.
< sipa>
morcos: the messy situation is a result of: satoshi, who wrote unintelligable but working code with relatively smart but ugly locking design that was not written down anywhere; other people took over who didn't understand that design, and race conditions appears; we got scared and added locking everywhere to be sure; now we're slowly trying to push locks down to improve performance
< morcos>
so if we're pushing locks down, shouldn't the lock be in CheckSequenceLocks?
< sipa>
ah, are there no callers for CheckSequenceLocks that already have the lock?
< morcos>
well, there is, removeForReorg. let me see if thats even necessary though
< morcos>
yeah i guess that is necessary b/c it iterates the mempool
< sipa>
you can add a tiny wrapper around CheckSequenceLocks inside the test code that grabs the lock
< morcos>
ok i don't want to waste your time, i just wanted to help move things in the right direction..
< sipa>
i don't really care :)
< morcos>
i'll just throw it at the top for now.. the tests are already broken with addUnchecked sitting there anyway, we can fix it when its a problem
< sipa>
yeah
< morcos>
oh that grabs it... hmm ok i'll do the wrapper..
< GitHub165>
bitcoin/0.12 c3faf78 instagibbs: Changed getnetworkhps value to double to avoid overflow....
< wumpus>
there we go, time for rc5...
< Luke-Jr>
wumpus: having two sets of settings is far uglier than writing bitcoin.conf IMO
< wumpus>
I don't agree, but we've already had this argument before
< Luke-Jr>
wumpus: any chance that would change if I'm looking at putting like 20 more options in there?
< wumpus>
well I don't want the daemon nor GUI writing to bitcoin.conf. There should be no assumption that the configuration file is writing at all
< wumpus>
writable*
< Luke-Jr>
how about a separate .conf writable loaded alongside bitcoin.conf?
< wumpus>
the number of options doesn't change that
< wumpus>
what's wrong with the qsettings?
< wumpus>
it is like a separate .conf writable alongside bitcoin.conf, except that it is the OS-sanctified way of storing application settings
< Luke-Jr>
and the var names are all different
< Luke-Jr>
and it won't get picked up on bitcoind
< wumpus>
yes, the difference in naming is ugly, that could be solved with some kind of mapping for old settings + use the same name for new settings
< wumpus>
that naming is inherited from satoshi-ism, the old place for the settings used to be the wallet(!)
< Luke-Jr>
heh
< Luke-Jr>
wumpus: and no value in bitcoind using the same settings? ;)
< wumpus>
and remember that whatever you do has to be backwards compatible, so what you imply is to add *another* source of settings. The initialization code (making sure that the option settings take precedence in the right order) is already complex enough. I'd really prefer not to tweak with it
< wumpus>
we have enough actual issues to fix...
< Luke-Jr>
well, not really another. I'd just load two conf files. :p
< Luke-Jr>
and start with just the new settings (which are mostly policy-related) in the new one
< GitHub120>
bitcoin/0.12 7728635 Wladimir J. van der Laan: doc: fix author list in release notes
< Luke-Jr>
sipa: half-sarcasm.
< sipa>
ha
< Luke-Jr>
it's a bug I absolutely need fixed to do Gentoo packaging, but I can throw it in with the system library stuff
< wumpus>
build system changes that don't influence the executable don't really warrant a new rc
< Luke-Jr>
it might. I noticed there are weird circumstances over when the 'g' prefixes the commit hash or not
< wumpus>
then again, you still have to fight it out with cfields what the genbuild.sh check should do at all, no way that will make 0.12.0 in teh first place :)
< Luke-Jr>
I didn't think it was worth looking into further at the time
< wumpus>
I don't think so either, it works ok, any well-meant change will probably break it in some way again
< wumpus>
are we really at the point that we need a test suite for the build system :p
< Luke-Jr>
I mean the build difference
< Luke-Jr>
I was wondering the other day..
< Luke-Jr>
how can we add a test suite for the test suite? :P
< wumpus>
lol
< sipa>
seems we need a recursive test system
< sipa>
which is able to test itself
< OxADADA>
we should also have 100% coverage of the test of the tests
< OxADADA>
:P
< wumpus>
quine test
< OxADADA>
my favorite test runner is the one that when detecting errors, deletes the offending function from the source code; recursively until no error is thrown.
< sipa>
what if the error is in main() ?
< OxADADA>
sipa: it will delete main()
< paveljanik>
it generates new one.
< sipa>
which will result in a linker error :p
< sipa>
complaining about the lack of main()
< OxADADA>
the most bug-free line of code is the line that was never written.
< wumpus>
I certainly prefer pulls that delete code to those that add code :)
< morcos>
wumpus: on that note and Luke-Jr this is mostly directed at you. are you guys ok with deleting the free transaction code from AcceptToMemoryPool then? I agree deleting from wallet is a first step, and maybe in an ideal world would have happened in an earlier release. But it seems to me by the time 0.13 is released its a lost cause anwyay.
< morcos>
Luke-Jr: what do you think if I create a PR to remove sending free transactions first (i suspect it'll probably be too big to back port to 0.12.1 though) and then follow on with a mempool cleanup.
< Luke-Jr>
morcos: IMO one of the conditions must first be met 1) removal from wallet in prior release; 2) de facto never working for the forseeable future
< morcos>
yeah i think any reasonable adoption of 0.12 is going to cause it to just not work...
< Luke-Jr>
although I would prefer maintaining relay of non-free txns as long as possible
< wumpus>
I tend to agree with Luke-Jr, sending them should be gone (or at least heavily discouraged) for a while then
< morcos>
wumpus: its been defaulted off for some time
< wumpus>
right
< Luke-Jr>
I still never use a fee myself FWIW
< morcos>
Ok, so lets break this into 2 parts
< Luke-Jr>
as long as I can successfully do so, I plan to re-add it to my own wallet ;)
< morcos>
Part 1) you're ok with removing the ability to send them?
< wumpus>
yeah, free transactions still seem to be working at this point
< Luke-Jr>
morcos: part 1, I prefer not to unless there is a good reason
< wumpus>
may take a long time to be included though, but not everyone is in a hurry
< Luke-Jr>
(expecting it to stop working before the next release would be a good reason)
< morcos>
well the reason i'm asking is i feel like cleaning up stuff like this towards the beginning of a release cycle makes other work easier
< Luke-Jr>
removing features is not really cleaning up. :p
< wumpus>
does it really make other work that much easier?
< morcos>
i find it hard to imagine its going to work. i think the size of your 0.12 mempool would need to be multiple GB's in order to not get full.
< morcos>
wumpus: yes, both in the wallet and in ATMP, you have to be careful around the special casing for free/priority stuff. Maybe thats only a limited area, but I seem to end up in that area a lot.
< Luke-Jr>
I think we'll be in a much better place to discuss this once 0.12 is released for a while
< Luke-Jr>
right now there's just too much to speculate on
< morcos>
okey dokey (is that what I'm supposed to say)
< sipa>
haha
< morcos>
Luke-Jr: alright related question
< morcos>
do you care about priority estimation
< Luke-Jr>
as things stand right now, I prefer gratis transactions to continue working as long as possible, but I won't fight removal of it like I am with priority-mining ;)
< Luke-Jr>
morcos: no
< morcos>
i think you are right that until and unless miners stop doing priority mining, fee estimation has the same problem with priority txs
< Luke-Jr>
priority is a fallback and anti-spam measure, it shouldn't be relied on for more than that IMO
< morcos>
but i want to decide if maintaining the priority estimation code is worth the benefit it has to fee estimation. its unclear at this point.
< morcos>
ok, so if improving fee estimation results in the removal of priority estimation, no big complaints from anyone?
< Luke-Jr>
I wasn't aware we had priority estimation code XD
< Luke-Jr>
unless you mean the priority display in coin control, which I find handy, but could live without if there's a benefit to removing it
< sipa>
Luke-Jr: so what do you think the Bitcoin Core wallet should do in the near future? always use fee estimation, and pay the estimated/chosen fee?
< morcos>
Luke-Jr: hmm.. good point, that used to return some hardcoded value, i think now it returns something based off priority estimation
< Luke-Jr>
actually, I'd probably hack it back in..
< Luke-Jr>
sipa: by default at least.
< Luke-Jr>
sipa: (this is already the case)
< morcos>
sipa: i'm confused by the question. isn't that what it currently does?
< Luke-Jr>
morcos: I did much prefer the pre-estimation priority estimates in coin control, actually
< Luke-Jr>
morcos: found it strange that priority changed with the slider..
< morcos>
Luke-Jr: i think it was a small change, might be easy to go back... but i'm not going to remove priority estimation unless i significantly rework the estimation code which is unknown right now.
< sipa>
morcos: i mean: should paying free transactions still be supported?
< sipa>
i never use Qt; i don't actually know what the sending dialog looks like currently
< morcos>
Well thats what I was starting by asking him about. Right now you can checkbox to sendfree if possible
< Luke-Jr>
brb
< morcos>
Luke-Jr and wumpus would like to not remove that yet, but are open to doing it for 0.13 if they become useless anyway due to limited mempools (which seems likely IMO)
< sipa>
that seems reasonable
< sipa>
but at least removing the priority estimation code could be done noe
< Luke-Jr>
note: also much less likely to be controversial if we wait until it breaks
< morcos>
well, its a bit circular
< wumpus>
right
< morcos>
its actually the priority estimation code that stops you from doing something stupid and trying to send a free tx when your own mempool is limited
< morcos>
it also won't let you send a free tx with priority less than that returned from the estimates
< wumpus>
I don't think supporting sending free transactions, for people that really want to and understand the risks, is so much of a maintenance burden
< morcos>
which haven't quite disappeared entirely yet
< wumpus>
and it is already not the default, you could hide the checkbox further, etc
< morcos>
wumpus: I think the UI in the QT makes it a little too easy to try to do that
< morcos>
yep
< wumpus>
but people seem to understand it
< jtimon>
why remove free transaction? not being default? obviously. a warning message discouraging it? why not?, but why prohibit the functionality forcing people like luke to remove the unwanted restriction?
< sipa>
you could use the coin control dialog is you really want to create a gratis transaction
< morcos>
well except they are about to become way way more unreliable
< wumpus>
everyone seems to intuitively know that they need to attach a fee to get transactions confirm
< wumpus>
jtimon: yeah, exactly
< wumpus>
jtimon: seems too much user babysitting
< morcos>
i guess i'm confused as to what the purpose of them is since the current design of mempools doesn't relay them 99% of the time
< sipa>
i care much more about the complexity of priority sorting, priority-based relay acceptance, and priority estimation than about the mere functionality of free transactions
< Luke-Jr>
sipa: coin control does not at this time allow overriding fees
< Luke-Jr>
adding it there makes sense though
< wumpus>
it could be moved there
< morcos>
sipa: aren't those all intertwined. if your own mempool won't accept it, its bad to let you send it
< morcos>
it just ties up your inputs
< Luke-Jr>
there used to be a "no forced fees" fork. if we just allow fee overriding in coin control, we can satisfy those people too :p
< jtimon>
are free transactions currently the default for the wallet?
< wumpus>
jtimon: no
< jtimon>
then at most I would just discourage them
< wumpus>
yes
< morcos>
wumpus: perhaps a valuable UI fix would be to make the checkbox clear after each tx or something. the fact that the setting gets saved is a bit disturbing
< wumpus>
agreed. on the other hand that's what people expect now, changing it is also going to disturb some people :(
< jtimon>
like sipa says, the hard part is the priority/separated space (not having a uniform metric to order all transactions)
< morcos>
so just so we're on the same page here
< Luke-Jr>
replacing it with a fee override in coin control seems ideal to me now IMO
< wumpus>
yes if you move the checkbox too you can change its behavior
< morcos>
you want to preserve the ability to send free txs, so that if you start your node and quickly send a tx before your mempool fills up you can send one (even though it likely won't propagate far)
< Luke-Jr>
fee override can ignore whether it's "safe" or not, so you don't need to calculate that anymore
< morcos>
it just seems a bit crazy to me that you can only do it in this race condition
< Luke-Jr>
morcos: it's possible the spam stops after >1 MB blocks
< Luke-Jr>
some of it anyway
< wumpus>
it's more of a philosophical issue, people don't want to be forced by their software to do something, even if it doesn't really make sense
< Luke-Jr>
also, the new spam filtering based on descendents can help
< morcos>
ok, well i just realized it might stop working entirely anyway
< jtimon>
morcos: who cares? don't use the functionality if you think it's stupid, just like I don't "echo asdgsdfgh > /dev/null" anymore
< morcos>
it used to be the case that if you couldn't get priority estimates you defaulted to allowminfree priority
< Luke-Jr>
it will probably be a long time before non-spam can fill mempools
< morcos>
this caused people who hadn't gotten estimates yet to send free txs with way too low priorities
< morcos>
so i changed it so that if you have no priority estimates, it requires infinite priority (so you can't send free txs)
< morcos>
i believe, although i can't be sure, that once 0.12 is rolled out in enough places, priority estimates will not have enough data and will always return -1
< morcos>
since that state is saved, it won't matter what you do with restarting your node or your mempool, your node won't let you send a free tx
< Luke-Jr>
morcos: I'm curious why you seem so sure that mempools will be full
< morcos>
although arguably thats exactly what we want? if you can configure your node in such a way that it receives enough data points to see free txs still being mined, it'll let you send one
< morcos>
so maybe that removes the concern of eliminating the sending ability a cycle before , and now we just have to wait and see what happens, and if it turns out no one can ever send them (bc limited mempools), then we can consider removing relay of them
< morcos>
Luke-Jr: it depends on your minrelayfee i suppose. certainly with 1000 sat/kB they fill up.
< Luke-Jr>
morcos: and your spam filtering capabilities
< morcos>
I have a 0.12 node up and running for about a week now with a 2GB mempool, its filled up to 1.8G so far
< Luke-Jr>
weird
< morcos>
its all txs at the lowest fee rate.
< Luke-Jr>
I wonder if we have regressions; my 0.10 node never overflowed my 32-bit process space
< Luke-Jr>
and I didn't set a minrelayfee on it
< sipa>
we keep much larger utxo caches now
< sipa>
in 0.10 nearly every block would flush it (past ibd)
< morcos>
a related problem i discussed with gmaxwell a couple days ago is we need to stop inving all these txs that will fall below most nodes effective min
< morcos>
i was thinking of implementing a p2p feefilter message
< paveljanik>
morcos, can you please post btc getrawmempool true | grep '"fee"' | sort | uniq -c | sort -rn | head from such node?
< morcos>
paveljanik: sure but i'm outputting more detailed stats, what would you like to know
< Luke-Jr>
0.10's minrelaytxfee is 5000
< paveljanik>
I'd like to see some oldest transactions coming from the most frequently used fee...
< paveljanik>
is it the remnant from the spam in July?
< paveljanik>
hmm, I can maybe try it myself on some node.
< paveljanik>
but this is the same I investigated in Dec.
< paveljanik>
15000 and 192 satoshis
< paveljanik>
Thank you
< morcos>
anyway, idea of a feefilter is you tell other nodes not even to bother inv'ing you stuff below the feerate your accepting now
< morcos>
would save a lot of inv bandwidth
< morcos>
but only works in the situation that you're not ALSO accepting free txs below your min fee rate (which is the case if your fee rate is caused by mempool limiting)
< Luke-Jr>
weird, when the same setting is set multiple times in bitcoin.conf, it's the first that has effect