Age | Commit message (Collapse) | Author |
|
|
|
This new node policy setting enables to accept replaced-by-fee
transaction without inspection of the replaceability signaling
as described in BIP125 "explicit signaling".
If turns on, the node mempool accepts transaction replacement
as described in `policy/mempool-replacements.md`.
The default setting value is `false`, implying opt-in RBF
is enforced.
|
|
Instead of duplicate the exact same code twice.
|
|
|
|
|
|
subtracting fees from output
140d942634f9f1bba191aafa948df57812c0f3fe wallet: don't add change fee to target if subtracting fees from output (S3RK)
Pull request description:
Change fee is payed by the recipient, so we don't need to add it to our target for coin selection.
ACKs for top commit:
achow101:
ACK 140d942634f9f1bba191aafa948df57812c0f3fe
ishaanam:
ACK 140d942634f9f1bba191aafa948df57812c0f3fe
furszy:
Code review ACK 140d9426
Tree-SHA512: b5efd0264c47ecee9204a3fd039bad24c69f9e614c6e1d9bb240ee5be6356b175aa074f3be123e6cfb8becd4d7bd1028eebe18801662cc69d19413d8d5a9dd5c
|
|
|
|
Instead of calling GetCachableAmount twice, which will result in
iterating through all the transaction txins/txouts and calling
GetDebit/GetCredit (which lock cs_wallet), just merge the filters and do
it once.
|
|
|
|
|
|
|
|
|
|
|
|
|
|
flight to the same peer
99f4785cad94657dcf349d00fdd6f1d44cac9bb0 Replace GetTime() with NodeClock in MaybeSendGetHeaders() (Suhas Daftuar)
abf5d16c24cb08b0451bdbd4d1de63a12930e8f5 Don't send getheaders message when another request is outstanding (Suhas Daftuar)
ffe87db247b19ffb8bfba329c5dd0be39ef5a53f Cleanup received_new_header calculation to use WITH_LOCK (Suhas Daftuar)
6d95cd3e7444ebaaabb64a76783ea3551530f1d7 Move peer state updates from headers message into separate function (Suhas Daftuar)
2b341db731793844f12944363186edea23eabdeb Move headers direct fetch to end of ProcessHeadersMessage (Suhas Daftuar)
29c45185223441943ab610e62937a118c7c3a5b2 Move headers-direct-fetch logic into own function (Suhas Daftuar)
bf8ea6df75749c27f753b562c4724b3f8d263ad4 Move additional headers fetching to own function (Suhas Daftuar)
9492e93bf9f4a841bf43ca4b593871c0863d5b63 Add helper function for checking header continuity (Suhas Daftuar)
7f2450871b3ea0b4d02d56bd2ca365fcc25cf90e Move handling of unconnecting headers into own function (Suhas Daftuar)
Pull request description:
Change `getheaders` messages so that we wait up to 2 minutes for a response to a prior `getheaders` message before issuing a new one.
Also change the handling of the `getheaders` message sent in response to a block INV, so that we no longer use the hashstop variable (including the hash stop will just mean that if our peer's headers chain is longer, then we won't learn
it, so there's no benefit to using hashstop).
Also, now respond to a `getheaders` during IBD with an empty headers message (rather than nothing) -- this better conforms to the intent of the new logic that it's better to not ignore a peer's `getheaders` message, even if you have nothing to give. This also avoids a lot of functional tests breaking.
This PR also reworks the headers processing logic to make it more readable.
ACKs for top commit:
ajtowns:
ACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0 ; code review, check over new logic of when to send getheaders messages
dergoegge:
Code review ACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0
mzumsande:
Code Review ACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0
sipa:
utACK 99f4785cad94657dcf349d00fdd6f1d44cac9bb0
w0xlt:
tACK https://github.com/bitcoin/bitcoin/pull/25454/commits/99f4785cad94657dcf349d00fdd6f1d44cac9bb0 Good improvement in the code.
Tree-SHA512: b8a63f6f71ac83e292edc0200def7835ad8b06b2955dd34e3ea6fac85980fa6962efd31d689ef5ea121ff5477ec14aafa4bbe2d0db134c05f4a31a57a8ced365
|
|
|
|
|
|
|
|
|
|
|
|
co-authored-by: Fabian Jahr <fjahr@protonmail.com>
|
|
co-authored-by: Fabian Jahr <fjahr@protonmail.com>
|
|
|
|
18f5355f3ad8a5513c99c5b0ca14266b604362cc Remove outdated comment (Igor Bubelov)
Pull request description:
Looks like this comment is no longer relevant, the last files which matched `index/*.cpp` pattern were removed in https://github.com/bitcoin/bitcoin/commit/f1006875665ffe8ff5da8185effe25b860743b4e
ACKs for top commit:
dongcarl:
ACK 18f5355f3ad8a5513c99c5b0ca14266b604362cc
shaavan:
ACK 18f5355f3ad8a5513c99c5b0ca14266b604362cc
Tree-SHA512: d3fcc2db0940f81ce521fddff836f271709ea327c357942383f8aff6c7089e74490fa720e7a2916900215c733d2b64960c1aa185f0c9b1567fce90a249d405e0
|
|
specific block with `use_index=false`
27c8056885b05bd25f540dbf6ede89230d43c7b9 rpc: Disallow gettxoutsetinfo queries for a specific block with use_index=false (Martin Zumsande)
Pull request description:
In the `gettxoutsetinfo` RPC, if we set `use_index` to false but specify `hash_or_height`, we currently hit a nonfatal error, e.g. `gettxoutsetinfo "muhash" "1" "false"` results in:
```
Internal bug detected: "!pindex || pindex->GetBlockHash() == view->GetBestBlock()"
rpc/blockchain.cpp:836 (GetUTXOStats)
```
The failing check was added in [#24410](https://github.com/bitcoin/bitcoin/pull/24410/commits/664a14ba7ccb40aa82d35a59831acd35db1897a6), but the previous behavior, returning the specified height together with data corresponding to the tip's height, was very confusing too in my opinion.
Fix this by disallowing the interaction of `use_index=false` and `hash_or_height` and add a RPC help example with `-named` because users might ask themselves how to use the `use_index` flag witout hitting an error.
An alternative way would be to allow the interaction if the specified `hash_or_height` happens to correspond to the tip (which should then also be applied to the `HASH_SERIALIZED` check before). If reviewers would prefer that, please say so.
ACKs for top commit:
fjahr:
utACK 27c8056885b05bd25f540dbf6ede89230d43c7b9
shaavan:
ACK 27c8056885b05bd25f540dbf6ede89230d43c7b9
Tree-SHA512: 1d81c34eaa48c86134a2cf7193246d5de6bfd819d413c3b3fae9cb9290e0297a336111eeaecede2f0f020b0f9a181d240de0da4493e1b387fe63b8189154442b
|
|
|
|
|
|
'WalletBatch' is created
c318211ddd48d44dd81dded553afeee3bc41c89e walletdb: fix last client version update (furszy)
bda8ebe608e6572eaaf40cd28dab6954241c9b0d wallet: don't read db every time that a new WalletBatch is created (furszy)
Pull request description:
Found it while was working on #25297.
We are performing a db read operation every time that a new `WalletBatch` is created, inside the constructor, just to check if the client version field is inside the db or not.
As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached.
ACKs for top commit:
achow101:
ACK c318211ddd48d44dd81dded553afeee3bc41c89e
w0xlt:
reACK https://github.com/bitcoin/bitcoin/pull/25383/commits/c318211ddd48d44dd81dded553afeee3bc41c89e
Tree-SHA512: 7fb780c656e169e8eb21e7212242494a647f6506d6da2cca828703713d440d29c82bec9e7d2c410f37b49361226ccd80846d3eeb8168383d0c2a11d85d73bee2
|
|
Since TaprootBuilder has assertions for the depth and leaf versions, the
PSBT decoder should check these values before calling
TaprootBuilder::Add so that the assertions are not triggered on
malformed taproot trees.
|
|
e866f0d0666664885d4c15c79bf59cc59975887a [functional test] submitrawpackage RPC (glozow)
fa076515b07ac4b10b2134e323bf4f56be5996a8 [rpc] add new submitpackage RPC (glozow)
Pull request description:
It would be nice for LN/wallet/app devs to test out package policy, package RBF, etc., but the only interface to do so right now is through unit tests. This PR adds a `-regtest` only RPC interface so people can test by submitting raw transaction data. It is regtest-only, as it would be unsafe/confusing to create an actual mainnet interface while package relay doesn't exist.
Note that the functional tests are there to ensure the RPC interface is working properly; they aren't for testing policy itself. See src/test/txpackage_tests.cpp.
ACKs for top commit:
t-bast:
Tested ACK against eclair https://github.com/bitcoin/bitcoin/pull/24836/commits/e866f0d0666664885d4c15c79bf59cc59975887a
ariard:
Code Review ACK e866f0d0
instagibbs:
code review ACK e866f0d0666664885d4c15c79bf59cc59975887a
Tree-SHA512: 824a26b10d2240e0fd85e5dd25bf499ee3dd9ba8ef4f522533998fcf767ddded9f001f7a005fe3ab07ec95e696448484e26599803e6034ed2733125c8c376c84
|
|
|
|
|
|
Follow-up to #617. This applies translator strings to the
reset options confirmation dialog and also refactors the way we pass the
strings to the dialog in order to allow the comments to be applied.
Because the strings were being concatenated, we can not apply translator
comments to all of the relevant strings. What we want to do instead is
have a variable in which the translatable strings are appended to using
the QString append function. This satisfies the Qt translator engine and
the comments are then properly applied within the `extracomment` field
in the translation file.
|
|
28a28a0c5bfae1a4149c795676a3109fd614b22a Squashed 'src/minisketch/' changes from 7eeb778fef..47f0a2d26f (fanquake)
Pull request description:
Contains:
* https://github.com/sipa/minisketch/pull/65
* https://github.com/sipa/minisketch/pull/66
Required for #25493.
ACKs for top commit:
achow101:
ACK dc375e5ccee5599374dddc2f0141a44a67c363b8
hebasto:
ACK dc375e5ccee5599374dddc2f0141a44a67c363b8, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: fbcd6cdc551770ff67d1df65ab171ce43c9eb7e7668da76da5c5b06865ed550527abcff661741a86c1535018a85a165619ff94ae3e6c7a695374b6c4f843c5ca
|
|
25e4762ae7a09906020e62cd947e9f71300439cf wallet: more accurate tx_noinputs_size (S3RK)
Pull request description:
Rationale: more accurate non-input fee estimation for txs with >=253 inputs
ACKs for top commit:
laanwj:
Concept and code review ACK 25e4762ae7a09906020e62cd947e9f71300439cf
achow101:
ACK 25e4762ae7a09906020e62cd947e9f71300439cf
furszy:
Code review ACK 25e4762a. left a small nit.
Tree-SHA512: bda8fad725d32ad3e13c007fa56ddb6679ac1a32098ddb08d9a114054acfa681cb66cd703ac675297f731cb381b09067a99a4efa31320140bbdd03f0cfdc81af
|
|
|
|
af56d63eca8a246c02506c2aef7ea8a22e2d07bb Revert "bnb: exit selection when best_waste is 0" (Murch)
Pull request description:
This reverts commit 9b5950db8683f9b4be03f79ee0aae8a780b01a4b.
Waste can be negative. At feerates lower than long_term_feerate this
means that a waste of 0 may be a suboptimal solution and this causes the
search to exit prematurely.
Only when the feerate is equal to the long_term_feerate would achieving
a waste of 0 indicate that we have achieved an optimal solution,
because it would mean that the excess is 0. It seems unlikely
that this would ever occur outside of test cases, and even then we
should prefer solutions with more inputs over solutions with fewer
according to previous decisions—but solutions with more inputs are found
later in the branch exploration.
The "optimization" described in #18257 and implemented in #18262 is
therefore a premature exit on a suboptimal solution and should be reverted.
ACKs for top commit:
sipa:
utACK af56d63eca8a246c02506c2aef7ea8a22e2d07bb
S3RK:
utACK af56d63eca8a246c02506c2aef7ea8a22e2d07bb
achow101:
ACK af56d63eca8a246c02506c2aef7ea8a22e2d07bb
glozow:
utACK af56d63eca8a246c02506c2aef7ea8a22e2d07bb, agree it is incorrect to stop here unless we could rule out the possibility of a better solution with negative waste. `SelectCoinsBnB` doesn't know what long term feerate and effective feerate are (and probably shouldn't) so it's better to have no exit early condition at all.
Tree-SHA512: 470f1a49041a0042cb69d239fccac7512ace79871d43508b6e7f7a2f3aca3523930b16e00c5513b816d5fe078d9ab53e42b0a80fd3c3d48e6434f24c2b009077
|
|
4786 - I don't think this exists any more?
4805 - Is already defined (globally) in the MSVC project.
Dropped 4717 and 4804, as it seems they are no-longer supressing
anything.
See:
https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warnings-c4000-c5999
|
|
|
|
The moved parts can be reviewed with "--color-moved=dimmed-zebra".
The one-char changes can be reviewed with "--word-diff-regex=.".
|
|
`ArgsManager`
d1684beabe5b738c2cc83de83e1aaef11a761b69 fees: Pass in a filepath instead of referencing gArgs (Carl Dong)
9a3d825c30e8e6118d74a4e568744cb9d03f7f5d init: Remove redundant -*mempool*, -limit* queries (Carl Dong)
6c5c60c4124293d948735756f84efc85262ea66f mempool: Use m_limit for UpdateTransactionsFromBlock (Carl Dong)
9e93b1030182eff92ef91181e17c7dd498c7e164 node/ifaces: Use existing MemPoolLimits (Carl Dong)
38af2bcf358a72b9457d370282e57f4be1c5c849 mempoolaccept: Use limits from mempool in constructor (Carl Dong)
9333427014695ac235c96d48791098168dfdc9db mempool: Introduce (still-unused) MemPoolLimits (Carl Dong)
716bb5fbd31077bbe99d11a54d6c2c250afc8085 scripted-diff: Rename anc/desc size limit vars to indicate SI unit (Carl Dong)
1ecc77321deb61b9f6888e4e10752b9d972fd26e scripted-diff: Rename DEFAULT_MEMPOOL_EXPIRY to indicate time unit (Carl Dong)
aa9141cd8185cb7ad532bc16feb9d302b05d9697 mempool: Pass in -mempoolexpiry instead of referencing gArgs (Carl Dong)
51c7a41a5eb6fcb60333812c770d80227cf7b64d init: Only determine maxmempool once (Carl Dong)
386c9472c8764738282e6d163b42e15a8feda7ea mempool: Make GetMinFee() with custom size protected (Carl Dong)
82f00de7a6a60cbc9ad0c6e1d0ffb1bc70c49af5 mempool: Pass in -maxmempool instead of referencing gArgs (Carl Dong)
f1941e8bfd2eecc478c7660434b1ebf6a64095a0 pool: Add and use MemPoolOptions, ApplyArgsManOptions (Carl Dong)
0199bd35bb44e32ee0db9b51c9d1bd7518c26f19 fuzz/rbf: Add missing TestingSetup (Carl Dong)
ccbaf546a68d6cda8ed3efd0598c0e4121b366bb scripted-diff: Rename DEFAULT_MAX_MEMPOOL_SIZE to indicate SI unit (Carl Dong)
fc02f77ca604f0221171bfde3059b34f5d0fb1cd ArgsMan: Add Get*Arg functions returning optional (Carl Dong)
Pull request description:
This is part of the `libbitcoinkernel` project: #24303, https://github.com/bitcoin/bitcoin/projects/18
-----
As mentioned in the Stage 1 Step 2 description of [the `libbitcoinkernel` project](https://github.com/bitcoin/bitcoin/issues/24303), `ArgsManager` will not be part of `libbitcoinkernel`. Therefore, it is important that we remove any dependence on `ArgsManager` by code that will be part of `libbitcoinkernel`. This is the first in a series of PRs aiming to achieve this.
This PR removes `CTxMemPool+MempoolAccept`'s dependency on `ArgsManager` by introducing a `CTxMemPool::Options` struct, which is used to specify `CTxMemPool`'s various options at construction time.
These options are:
- `-maxmempool` -> `CTxMemPool::Options::max_size`
- `-mempoolexpiry` -> `CTxMemPool::Options::expiry`
- `-limitancestorcount` -> `CTxMemPool::Options::limits::ancestor_count`
- `-limitancestorsize` -> `CTxMemPool::Options::limits::ancestor_size`
- `-limitdescendantcount` -> `CTxMemPool::Options::limits::descendant_count`
- `-limitdescendantsize` -> `CTxMemPool::Options::limits::descendant_size`
More context can be gleaned from the commit messages. The important commits are:
- 56eb479ded8bfb2ef635bb6f3b484f9d5952c70d "pool: Add and use MemPoolOptions, ApplyArgsManOptions"
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 6f4bf3ede5812b374828f08fc728ceded2f10024 "mempool: Pass in -mempoolexpiry instead of referencing gArgs"
- 5958a7fe4806599fc620ee8c1a881ca10fa2dd16 "mempool: Introduce (still-unused) MemPoolLimits"
Reviewers: Help needed in the following commits (see commit messages):
- a1e08b70f3068f4e8def1c630d8f50cd54da7832 "mempool: Pass in -maxmempool instead of referencing gArgs"
- 0695081a797e9a5d7787b78b0f8289dafcc6bff7 "node/ifaces: Use existing MemPoolLimits"
Note to Reviewers: There are perhaps an infinite number of ways to architect `CTxMemPool::Options`, the current one tries to keep it simple, usable, and flexible. I hope we don't spend too much time arguing over the design here since that's not the point. In the case that you're 100% certain that a different design is strictly better than this one in every regard, please show us a fully-implemented branch.
-----
TODO:
- [x] Use the more ergonomic `CTxMemPool::Options` where appropriate
- [x] Doxygen comments for `ApplyArgsManOptions`, `MemPoolOptions`
-----
Questions for Reviewers:
1. Should we use `std::chrono::seconds` for `CTxMemPool::Options::expiry` and `CTxMemPool::m_expiry` instead of an `int64_t`? Something else? (`std::chrono::hours`?)
2. Should I merge `CTxMemPool::Limits` inside `CTxMemPool::Options`?
ACKs for top commit:
MarcoFalke:
ACK d1684beabe5b738c2cc83de83e1aaef11a761b69 🍜
ryanofsky:
Code review ACK d1684beabe5b738c2cc83de83e1aaef11a761b69. Just minor cleanups since last review, mostly switching to brace initialization
Tree-SHA512: 2c138e52d69f61c263f1c3648f01c801338a8f576762c815f478ef5148b8b2f51e91ded5c1be915e678c0b14f6cfba894b82afec58d999d39a7bb7c914736e0b
|
|
|
|
ac4fb3bbbe207d0744201a9df8a971af06db5476 gui: reset options, notify user about the backup creation (furszy)
Pull request description:
Quick follow-up to first point of https://github.com/bitcoin-core/gui/pull/602#pullrequestreview-1002780997
ACKs for top commit:
ryanofsky:
Code review ACK ac4fb3bbbe207d0744201a9df8a971af06db5476, just fixing displayed backup directory since last review
jarolrod:
tACK ac4fb3bbbe207d0744201a9df8a971af06db5476
Tree-SHA512: cfeca5cd6d6d3d69bbd81211cf1bfd490de13ac96bf53be081a5ceb88611afa57dff2be35f8e0a41b1088b7b892f75a21a9abf47f2e1d77e9e316467eb7c12be
|
|
by returning an RPC error where previously a NonFatalError
would be thrown.
|
|
This reverts commit 9b5950db8683f9b4be03f79ee0aae8a780b01a4b.
Waste can be negative. At feerates lower than long_term_feerate this
means that a waste of 0 may be a suboptimal solution and this causes the
search to exit prematurely.
Only when the feerate is equal to the long_term_feerate would achieving
a waste of 0 indicate that we have achieved an optimal solution,
because it would mean that the excess is 0. It seems unlikely
that this would ever occur outside of test cases, and even then we
should prefer solutions with more inputs over solutions with fewer
according to previous decisions—but solutions with more inputs are found
later in the branch exploration.
The "optimization" described in #18257 and implemented in #18262 is
therefore a premature exit on a suboptimal solution and should be reverted.
|
|
|
|
Now that MemPoolOptions has correctly-determined max_size and limits
members, perform sanity checks on that instead of re-determining the
options.
|
|
Change getheaders messages so that we wait up to 2 minutes for a response to a
prior getheaders message before issuing a new one.
Also change the handling of the getheaders message sent in response to a block
INV, so that we no longer use the hashstop variable (including the hash stop
will just mean that if our peer's headers chain is longer, then we won't learn
it, so there's no benefit to using hashstop).
Also, now respond to a getheaders during IBD with an empty headers message
(rather than nothing) -- this better conforms to the intent of the new logic
that it's better to not ignore a peer's getheaders message, even if you have
nothing to give. This also avoids a lot of functional tests breaking.
p2p_segwit.py is modified to use this same strategy, as the test logic (of
expecting a getheaders after a block inv) would otherwise be broken.
|
|
|
|
|