5d3f98d278 refactor: Replace m_params with chainman.GetParams() (Aurèle Oulès)
Pull request description:
Fixes a TODO introduced in #24595.
Removes `m_params` from `CChainState` class and replaces it with `m_chainman.GetParams()`.
ACKs for top commit:
MarcoFalke:
review ACK 5d3f98d278🌎
Tree-SHA512: de0fe31450d281cc7307c0d820495e86c93c7998e77a148db2c703da66cff1059e6560c041f1864913c42075aa24d259c2623d45e929ca0a8056ed330a9f9978
1c48dae76f test: Use C++11 member initializers for TestMemPoolEntryHelper (MacroFake)
fad7f2239c test: Remove unused txmempool include from tests (MacroFake)
Pull request description:
Seems odd to include this heavy header in all tests despite it only being used in a few tests.
Can be reviewed with `--color-moved=dimmed-zebra --ignore-all-space`
ACKs for top commit:
aureleoules:
reACK 1c48dae76f
hebasto:
ACK 1c48dae76f, I have reviewed the code and it looks OK, I agree it can be merged.
w0xlt:
ACK 1c48dae76f
Tree-SHA512: 31f2808d04ec33bfc2409832b8e59e6c870eaa98fbcf879e1c786492c7d07134711b30f8290bdb34e1b8f7b8f2f11dae8e10c64e7eb31f584b2f5c58fcc7743b
b147322a7a Use `PACKAGE_NAME` in messages rather than hardcoding "Bitcoin Core" (Hennadii Stepanov)
Pull request description:
Usually, we do not hardcode "Bitcoin Core" in the user-faced messages.
See:
- bitcoin/bitcoin#18646
- bitcoin/bitcoin#19282
Also grammar has been improved -- singular instead of plural.
ACKs for top commit:
jarolrod:
ACK b147322a7a
Tree-SHA512: b135c18703dfdd7b63d4cb27d1ac48f6a9dbf69382142ae381f33bf561cbf57477a11d1c73263aa834f705206d7dd5716df2523d38ed0d4cfec8babc38bb017a
This changes the flag for the bitcoin-chainstate executable. Previously
it was false, now it is the chain's default value (still false for the
main chain).
This changes the minimum chain work for the bitcoin-chainstate
executable. Previously it was uint256{}, now it is the chain's default
minimum chain work.
af781bf4b2 doc: fix typo in doc/libraries.md (fanquake)
9e9ae6101f doc: remove library commentary from src/Makefile.am (fanquake)
Pull request description:
Deduplicate the makefile comments, in favour of doc/libraries.md. I think a single, more comprehensive source of truth is preferable. Diagrams are also useful. Came up in https://github.com/bitcoin/bitcoin/pull/26292#issuecomment-1275094478.
ACKs for top commit:
ryanofsky:
Code review ACK af781bf4b2, nice cleanups
hebasto:
ACK af781bf4b2, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: df61ed1394102221701ae2dfa42886dfabe9d9fd7f601b794e2195f93d8f7c2a1cd1c000a77d0a969b42328e8ebc0387755c57291837b283fdf376dbd98fdda1
We optimistically pre-register a peer for txreconciliations
upon sending txreconciliation support announcement.
But if, at VERACK, we realize that the peer never sent
WTXIDRELAY message, we should unregister the peer
from txreconciliations, because txreconciliations rely on wtxids.
Once we received a reconciliation announcement support
message from a peer and it doesn't violate our protocol,
we store the negotiated parameters which will be used
for future reconciliations.
If we're connecting to the peer which might support
transaction reconciliation, we announce we want to reconcile
with them.
We store the reconciliation salt so that when the peer
responds with their salt, we are able to compute the
full reconciliation salt.
This behavior is enabled with a CLI flag.
e899d4ca6f init: limit bip30 exceptions to coinbase txs (Chris Geihsler)
511eb7fdea Ignore problematic blocks in DisconnectBlock (Chris Geihsler)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/22596
When using checklevel=4, block verification fails because of duplicate coinbase transactions involving blocks 91812 and 91722. There was already a check in place within `ConnectBlock` to ignore the problematic blocks, but `DisconnectBlock` did not contain a similar check to ignore these blocks when called from `VerifyDB`.
By ignoring these two blocks in `DisconnectBlock`, the block verification process succeeds at checklevel=4.
(Note to reviewers: this is my first contribution to Bitcoin Core, so any feedback is most welcome. Thanks in advance for reviewing!)
## Steps to reproduce:
Use the following bitcoin.conf file and start bitcoind. I only used block data through block ~100000 so that the verification process was much faster.
```
assumevalid=0
checkblocks=0
checklevel=4
```
Without this change, you will see the following error when the blocks are verified:
```
2022-04-14T02:56:44Z init message: Verifying blocks…
2022-04-14T02:56:44Z Verifying last 101881 blocks at level 4
2022-04-14T02:56:44Z [0%]...[10%]...[20%]...[30%]...[40%]...ERROR: VerifyDB(): *** coin database inconsistencies found (last 10160 blocks, 142571 good transactions before that)
2022-04-14T02:57:01Z : Corrupted block database detected.
Please restart with -reindex or -reindex-chainstate to recover.
: Corrupted block database detected.
Please restart with -reindex or -reindex-chainstate to recover.
```
With this change, you will see this instead:
```
2022-04-14T02:32:29Z init message: Verifying blocks…
2022-04-14T02:32:29Z Verifying last 101746 blocks at level 4
2022-04-14T02:32:29Z [0%]...[10%]...[20%]...[30%]...[40%]...[50%]...[60%]...[70%]...[80%]...[90%]...[DONE].
2022-04-14T02:32:48Z No coin database inconsistencies in last 101746 blocks (226126 transactions)
```
ACKs for top commit:
laanwj:
Code review ACK e899d4ca6f
achow101:
ACK e899d4ca6f
jamesob:
(Biased) ACK e899d4ca6f ([`jamesob/ackr/24851.2.seejee.init_ignore_bip_30_verif`](https://github.com/jamesob/bitcoin/tree/ackr/24851.2.seejee.init_ignore_bip_30_verif))
Tree-SHA512: d2f6d25e9619aee32c1a73fe846b1b587698eaa5a4994fa6424f1038f45654f9fd52b74a69843cc84d90168d74827130ccf8e9201502f5d52281acdb20429291
a8250e30f1 doc: add release note about `/rest/deploymentinfo` (brunoerg)
5c96020024 doc: add `/deploymentinfo` in REST-interface (brunoerg)
3e44bee08e test: add coverage for `/rest/deploymentinfo` (brunoerg)
91497031cb rest: add `/deploymentinfo` (brunoerg)
Pull request description:
#23508 added a new RPC named `getdeploymentinfo`, it moved the softfork section from `getblockchaininfo` into this new one. In the REST interface, we have an endpoint named`/rest/chaininfo.json` (which refers to `getblockchaininfo`), so, this PR adds a new REST endpoint named `/deploymentinfo` which refers to `getdeploymentinfo`.
You can use it by passing a block hash, e.g: '/rest/deploymentinfo/<BLOCKHASH>.json' or you can use it without passing a block hash to get the 'deploymentinfo' for the last block.
ACKs for top commit:
jonatack:
re-ACK a8250e30f1 rebase-only since my last review at c65f82bb
achow101:
ACK a8250e30f1
stickies-v:
re-ACK a8250e30f1
Tree-SHA512: 0735183b6828d51a72ed0e2be5a09b314ac4693f548982c6e9adaa0ef07a55aa428d3b2d1b1de70b83169811a663a8624b686166e5797f624dcc00178b9796e6
8173f160e0 style: rename variables to match coding style (Vasil Dimov)
8b4ad203d0 fees: make FeeFilterRounder::feeset const (Vasil Dimov)
e7a5bf6be7 fees: make the class FeeFilterRounder thread-safe (Vasil Dimov)
Pull request description:
Make the class `FeeFilterRounder` thread-safe so that its methods can be called concurrently by different threads on the same object. Currently it has just one method (`round()`).
The second commit is optional, but it improves readability, showing that the `feeset` member will never be changed, thus does not need protection from concurrent access.
ACKs for top commit:
jonatack:
re-ACK 8173f160e0
laanwj:
Code review ACK 8173f160e0
promag:
Code review ACK 8173f160e0
Tree-SHA512: 94b809997c485c0d114fa702d0406b980be8eaaebcfefa56808ed670aa943959c2f16cfd0ef72b4752fe2a409a23af1b4b7f2f236e51212957759569e3bbbefd
bfb9b94ebe wallet: remove duplicate descriptor type check in GetNewDestination (furszy)
76b982a4a5 wallet: remove unused `nAccountingEntryNumber` field (furszy)
599ff5adfc wallet: avoid double TopUp() calls on descriptor wallets (furszy)
Pull request description:
Found it while was digging over a `getnewaddress` timeout on the functional test suite.
### Context:
We are calling `TopUp()` twice in the following flows for descriptor wallets:
A) `CWallet::GetNewDestination`:
1) Calls spk_man->TopUp()
2) Calls spk_man->GetNewDestination() --> which, after the basic script checks, calls TopUp() again.
B) `CWallet::GetReservedDestination`:
1) Calls spk_man->TopUp()
2) Calls spk_man->GetReservedDestination() --> which calls to GetNewDestination (which calls to TopUp again).
### Changes:
Move `TopUp()` responsibility from the wallet class to each scriptpubkeyman.
So each spkm can decide to call it or not after perform the basic checks
for the new destination request.
Aside from that, remove the unused `nAccountingEntryNumber` wallet field. And a duplicated descriptor type check in `GetNewDestination`
ACKs for top commit:
aureleoules:
re-ACK bfb9b94ebe.
achow101:
ACK bfb9b94ebe
theStack:
Code-review ACK bfb9b94ebe
Tree-SHA512: 3ab73f37729e50d6c6a4434f676855bc1fb404619d63c03e5b06ce61c292c09c59d64cb1aa3bd9277b06f26988956991d62c90f9d835884f41ed500b43a12058
a3789c700b Improve getpeerinfo pingtime, minping, and pingwait help docs (Jon Atack)
df660ddb1c Update getpeerinfo/-netinfo/TxRelay#m_relay_txs relaytxes docs (for v24 backport) (Jon Atack)
1f448542e7 Always return getpeerinfo "minfeefilter" field (for v24 backport) (Jon Atack)
9cd6682545 Make getpeerinfo field order consistent with its help (for v24 backport) (Jon Atack)
Pull request description:
Various updates and fixups, mostly targeting v24. Please refer to the commit messages for details.
ACKs for top commit:
achow101:
ACK a3789c700b
brunoerg:
ACK a3789c700b
vasild:
ACK a3789c700b
Tree-SHA512: b8586a9b83c1b18786b5ac1fc1dba91573c13225fc2cfc8d078f4220967c95056354f6be13327f33b4fcf3e9d5310fa4e1bdc93102cbd6574f956698993a54bf
626b7c8493 fuzz: add scanblocks as safe for fuzzing (James O'Beirne)
94fe5453c7 test: rpc: add scanblocks functional test (Jonas Schnelli)
6ef2566b68 rpc: add scanblocks - scan for relevant blocks with descriptors (Jonas Schnelli)
a4258f6e81 rpc: move-only: consolidate blockchain scan args (James O'Beirne)
Pull request description:
Revives #20664. All feedback from the previous PR has either been responded to inline or incorporated here.
---
Major changes from Jonas' PR:
- consolidated arguments for scantxoutset/scanblocks
- substantial cleanup of the functional test
Here's the range-diff (`git range-diff master jonasschnelli/2020/12/filterblocks_rpc jamesob/2021-11-scanblocks`): https://gist.github.com/jamesob/aa4a975344209f0316444b8de2ec1d18
### Original PR description
> The `scanblocks` RPC call allows one to get relevant blockhashes from a set of descriptors by scanning all blockfilters in a given range.
>
> **Example:**
>
> `scanblocks start '["addr(<bitcoin_address>)"]' 661000` (returns relevant blockhashes for `<bitcoin_address>` from blockrange 661000->tip)
>
> ## Why is this useful?
> **Fast wallet rescans**: get the relevant blocks and only rescan those via `rescanblockchain getblockheader(<hash>)[height] getblockheader(<hash>)[height])`. A future PR may add an option to allow to provide an array of blockhashes to `rescanblockchain`.
>
> **prune wallet rescans**: (_needs additional changes_): together with a call to fetch blocks from the p2p network if they have been pruned, it would allow to rescan wallets back to the genesis block in pruned mode (relevant #15946).
>
> **SPV mode** (_needs additional changes_): it would be possible to build the blockfilterindex from the p2p network (rather then deriving them from the blocks) and thus allow some sort of hybrid-SPV mode with moderate bandwidth consumption (related #9483)
ACKs for top commit:
furszy:
diff re-ACK 626b7c8
Tree-SHA512: f84e4dcb851b122b39e9700c58fbc31e899cdcf9b587df9505eaf1f45578cc4253e89ce2a45d1ff21bd213e31ddeedbbcad2c80810f46755b30acc17b07e2873
bf95976061 doc: add note about snapshot chainstate init (James O'Beirne)
e4d7995286 test: add testcases for snapshot initialization (James O'Beirne)
cced4e7336 test: move-only-ish: factor out LoadVerifyActivateChainstate() (James O'Beirne)
51fc9241c0 test: allow on-disk coins and block tree dbs in tests (James O'Beirne)
3c361391b8 test: add reset_chainstate parameter for snapshot unittests (James O'Beirne)
00b357c215 validation: add ResetChainstates() (James O'Beirne)
3a29dfbfb2 move-only: test: make snapshot chainstate setup reusable (James O'Beirne)
8153bd9247 blockmanager: avoid undefined behavior during FlushBlockFile (James O'Beirne)
ad67ff377c validation: remove snapshot datadirs upon validation failure (James O'Beirne)
34d1590331 add utilities for deleting on-disk leveldb data (James O'Beirne)
252abd1e8b init: add utxo snapshot detection (James O'Beirne)
f9f1735f13 validation: rename snapshot chainstate dir (James O'Beirne)
d14bebf100 db: add StoragePath to CDBWrapper/CCoinsViewDB (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: https://github.com/bitcoin/bitcoin/pull/15606)
---
Half of the replacement for #24232. The original PR grew larger than expected throughout the review process.
This change adds the ability to initialize a snapshot-based chainstate during init if one is detected on disk. This is of course unused as of now (aside from in unittests) given that we haven't yet enabled actually loading snapshots.
Don't be scared! There are some big move-only commits in here.
Accompanying changes include:
- moving the snapshot coinsdb directory from being called `chainstate_[base blockhash]` to `chainstate_snapshot`, since we only support one snapshot in use at a time. This simplifies some logic, but it necessitates writing that base blockhash out to a file within the coinsdb dir. See [discussion here](https://github.com/bitcoin/bitcoin/pull/24232#discussion_r832762880).
- adding a simple fix in `FlushBlockFile()` that avoids a crash when attemping to flush to disk before `LoadBlockIndexDB()` is called, which happens when calling `MaybeRebalanceCaches()` during multiple chainstate init.
- improving the unittest to allow testing with on-disk chainstates - necessary to test a simulated restart and re-initialization.
ACKs for top commit:
naumenkogs:
utACK bf95976061
ariard:
Code Review ACK bf9597606
ryanofsky:
Code review ACK bf95976061. Changes since last review: rebasing, switching from CAutoFile to AutoFile, adding comments, switching from BOOST_CHECK to Assert in test util, using chainman.GetMutex() in tests, destroying one ChainstateManager before creating a new one in tests
fjahr:
utACK bf95976061
aureleoules:
ACK bf95976061
Tree-SHA512: 15ae75caf19f8d12a12d2647c52897904d27b265a7af6b4ae7b858592eeadb8f9da6c2394b6baebec90adc28742c053e3eb506119577dae7c1e722ebb3b7bcc0
9e386afb67 tests: Test that PSBT_OUT_TAP_TREE is included correctly (Andrew Chow)
30ff25cf37 psbt: Only include m_tap_tree if it has scripts (Andrew Chow)
0577d423ad psbt: Change m_tap_tree to store just the tuples (Andrew Chow)
22c051ca70 tests: Test that PSBT_OUT_TAP_TREE is combined correctly (Andrew Chow)
7df6e1bb77 psbt: Fix merging of m_tap_tree (Andrew Chow)
0652dc53b2 [BugFix]: Do not allow deserializing PSBT with empty PSBT_OUT_TAP_TREE (Jeremy Rubin)
Pull request description:
PSBT_OUT_TAP_TREE should not be included for outputs that do not have such a tree. This should be disallowed during parsing, as well as prior to serialization when the field is populated during updating.
Also added some test cases.
Alternative to #25856
ACKs for top commit:
instagibbs:
ACK 9e386afb67
darosior:
ACK 9e386afb67
Tree-SHA512: ce5c02a69752d176dbd967c1e8d30129b1905c8f186aeeef034576c1de82059271a1ee846bd040f5be4e66bb77ba711dcf14ac1e597c5707d7e7e2293f6cfefb
b01682a812 refactor: revert m_next_resend to not be std::atomic (stickies-v)
9245f45670 wallet: only update m_next_resend when actually resending (stickies-v)
7fbde8af5c refactor: carve out tx resend timer logic into ShouldResend (stickies-v)
01f3534632 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v)
c6e8e11fb0 wallet: fix capitalization in docstring (stickies-v)
Pull request description:
This PR addresses the outstanding comments/issues from #25768:
- capitalization [typo](https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958572522) in docstring
- remove [unused locks](01f3534632) that we previously needed for `ReacceptWalletTransactions()`
- before #25768, only `ResendWalletTransactions()` would reset `m_next_resend` (formerly called `nNextResend`). By unifying it with `ReacceptWalletTransactions()` into `ResubmitWalletTransactions()`, the number of callsites that would reset the `m_next_resend` timer increased
- since `m_next_resend` is only used in case of `relay=true` (formerly `ResendWalletTransactions()`), this is unintuitive
- it leads to [unexpected behaviour](https://github.com/bitcoin/bitcoin/pull/25768#issuecomment-1252619427) such as transactions potentially never being rebroadcasted.
- it makes the ResubmitWalletTransactions()` logic [more complicated than strictly necessary](https://github.com/bitcoin/bitcoin/pull/25768#discussion_r962828563)
- since #25768, we relied on an earlier call of `ResubmitWalletTransactions(relay=false, force=true)` to initialize `m_next_resend()`, I think we can more elegantly do that by just providing `m_next_resend` with a default value
- just to highlight: this commit introduces behaviour change
Note: the `if (!fBroadcastTransactions)` in `CWallet:ShouldResend()` is duplicated on purpose, since it potentially avoids the slightly more expensive `if (!chain().isReadyToBroadcast())` check afterwards. I don't have a strong view on it, so happy to remove that additional check to reduce the diff, too.
ACKs for top commit:
aureleoules:
ACK b01682a812
achow101:
ACK b01682a812
Tree-SHA512: ac5f1d8858f8dd736dd1480f385984d660c1916b62a42562317020e8f9fd6a30bd8f23d973d47e4c9480d744c5ba39fdbefd69568a5eb0589a8422d7e5971c1c
861cb3fadc test: move SyncWithValidationInterfaceQueue() before Stop() in txindex_tests (Vasil Dimov)
6526dc3b78 test: silence TSAN false positive in coinstatsindex_initial_sync (Vasil Dimov)
Pull request description:
Silence false positives from TSAN about unsynchronized calls to `BaseIndex::~BaseIndex()` and `BaseIndex::SetBestBlockIndex()`. They are synchronized, but beyond the comprehension of TSAN - by `SyncWithValidationInterfaceQueue()`, called from `BaseIndex::BlockUntilSyncedToCurrentChain()`.
Fixes https://github.com/bitcoin/bitcoin/issues/25365
ACKs for top commit:
MarcoFalke:
review ACK 861cb3fadc
ryanofsky:
Code review ACK 861cb3fadc. Just comment change since last review.
Tree-SHA512: 8c30fdf2fd11d54e9adfa68a67185ab820bd7bd9f7f3ad6456e7e6d219fa9cf6d34b41e98e723eae86cb0c1baef7f3fc57b1b011a13dc3fe3d78334b9b5596de
b527b54950 net: convert standalone SetSocketNonBlocking() to Sock::SetNonBlocking() (Vasil Dimov)
29f66f7682 moveonly: move SetSocketNonBlocking() from netbase to util/sock (Vasil Dimov)
b4bac55679 net: convert standalone IsSelectableSocket() to Sock::IsSelectable() (Vasil Dimov)
5db7d2ca0a moveonly: move IsSelectableSocket() from compat.h to sock.{h,cpp} (Vasil Dimov)
Pull request description:
_This is a piece of #21878, chopped off to ease review._
* convert standalone `IsSelectableSocket()` to `Sock::IsSelectable()`
* convert standalone `SetSocketNonBlocking()` to `Sock::SetNonBlocking()`
This further encapsulates syscalls inside the `Sock` class and makes the callers mockable.
ACKs for top commit:
jonatack:
ACK b527b54950 review/debug build/unit tests at each commit, cross-referenced the changes with `man select` and `man errno`, ran a signet node on the last commit with ip4/ip6//tor/i2p/cjdns and network connections were nominal
dergoegge:
Code review ACK b527b54950
Tree-SHA512: af783ce558c7a89e173f7ab323fb3517103d765c19b5d14de29f64706b4e1fea3653492e8ea73ae972699986aaddf2ae72c7cfaa7dad7614254283083b7d2632
bcb0cacac2 reindex, log, test: fixes #21379 (mruddy)
Pull request description:
Fixes #21379.
The blocks/blk?????.dat files are mutated and become increasingly malformed, or corrupt, as a result of running the re-indexing process.
The mutations occur after the re-indexing process has finished, as new blocks are appended, but are a result of a re-indexing process miscalculation that lingers in the block manager's `m_blockfile_info` `nSize` data until node restart.
These additions to the blk files are non-fatal, but also not desirable.
That is, this is a form of data corruption that the reading code is lenient enough to process (it skips the extra bytes), but it adds some scary looking log messages as it encounters them.
The summary of the problem is that the re-index process double counts the size of the serialization header (magic message start bytes [4 bytes] + length [4 bytes] = 8 bytes) while calculating the blk data file size (both values already account for the serialization header's size, hence why it is over accounted).
This bug manifests itself in a few different ways, after re-indexing, when a new block from a peer is processed:
1. If the new block will not fit into the last blk file processed while re-indexing, while remaining under the 128MiB limit, then the blk file is flushed to disk and truncated to a size that is 8 greater than it should be. The truncation adds zero bytes (see `FlatFileSeq::Flush` and `TruncateFile`).
1. If the last blk file processed while re-indexing has logical space for the new block under the 128 MiB limit:
1. If the blk file was not already large enough to hold the new block, then the zeros are, in effect, added by `fseek` when the file is opened for writing. Eight zero bytes are added to the end of the last blk file just before the new block is written. This happens because the write offset is 8 too great due to the miscalculation. The result is 8 zero bytes between the end of the last block and the beginning of the next block's magic + length + block.
1. If the blk file was already large enough to hold the new block, then the current existing file contents remain in the 8 byte gap between the end of the last block and the beginning of the next block's magic + length + block. Commonly, when this occcurs, it is due to the blk file containing blocks that are not connected to the block tree during reindex and are thus left behind by the reindex process and later overwritten when new blocks are added. The orphaned blocks can be valid blocks, but due to the nature of concurrent block download, the parent may not have been retrieved and written by the time the node was previously shutdown.
ACKs for top commit:
LarryRuane:
tested code-review ACK bcb0cacac2
ryanofsky:
Code review ACK bcb0cacac2. This is a disturbing bug with an easy fix which seems well-worth merging.
mzumsande:
ACK bcb0cacac2 (reviewed code and did some testing, I agree that it fixes the bug).
w0xlt:
tACK bcb0cacac2
Tree-SHA512: acc97927ea712916506772550451136b0f1e5404e92df24cc05e405bb09eb6fe7c3011af3dd34a7723c3db17fda657ae85fa314387e43833791e9169c0febe51
fa08663344 rpc: Return coinbase flag in scantxoutset (MacroFake)
Pull request description:
I guess it can't hurt to return this for someone that wants to know it
ACKs for top commit:
aureleoules:
ACK fa08663344
shaavan:
ACK fa08663344
Tree-SHA512: 04c554b3ed9877bab93ffcf0c1a4430cd41b30c5f4f3bf462a518fc8b3d68832dd85a29e81bd805eaa16e987856933d7a888a8c126f670bb2844bbd5ca1bf902
04526787b5 Validate `port` options (amadeuszpawlik)
f8387c4234 Validate port value in `SplitHostPort` (amadeuszpawlik)
Pull request description:
Validate `port`-options, so that invalid values are rejected early in the startup.
Ports are `uint16_t`s, which effectively limits a port's value to <=65535. As discussed in https://github.com/bitcoin/bitcoin/pull/24116 and https://github.com/bitcoin/bitcoin/pull/24344, port "0" is considered invalid too.
Proposed in https://github.com/bitcoin/bitcoin/issues/21893#issuecomment-835784223
The `SplitHostPort(std::string in, uint16_t& portOut, std::string& hostOut)` now returns a bool that indicates whether the port value was set and within the allowed range. This is an improvement that can be used not only for port validation of options at startup, but also in rpc calls, etc,
ACKs for top commit:
luke-jr:
utACK 04526787b5
ryanofsky:
Code review ACK 04526787b5. Just suggested changes since last review: reverting some SplitHostPort changes, adding release notes, avoiding 'GetArgs[0]` problem.
Tree-SHA512: f1ac80bf98520b287a6413ceadb41bc3a93c491955de9b9319ee1298ac0ab982751905762a287e748997ead6198a8bb7a3bc8817ac9e3d2468e11ab4a0f8496d
75c3f9f880 sync: rename AnnotatedMixin::UniqueLock to AnnotatedMixin::unique_lock (Vasil Dimov)
8d9ee8efe8 sync: remove DebugLock alias template (Vasil Dimov)
4b2e16763f sync: avoid confusing name overlap (Mutex) (Vasil Dimov)
9d7ae4b66c sync: remove unused template parameter from ::UniqueLock (Vasil Dimov)
11c190e3f1 sync: simplify MaybeCheckNotHeld() definitions by using a template (Vasil Dimov)
Pull request description:
Summary:
* Reduce 4 of the `MaybeCheckNotHeld()` definitions to 2 by using a template.
* Remove unused template parameter from `::UniqueLock`.
* Use `MutexType` instead of `Mutex` for a template parameter name to avoid overlap/confusion with the `Mutex` class.
* Rename `AnnotatedMixin::UniqueLock` to `AnnotatedMixin::unique_lock` to avoid overlap/confusion with the global `UniqueLock` and for consistency with `UniqueLock::reverse_lock`.
The first commit `sync: simplify MaybeCheckNotHeld() definitions by using a template` is also part of https://github.com/bitcoin/bitcoin/pull/25390
ACKs for top commit:
aureleoules:
ACK 75c3f9f880 - LGTM
ryanofsky:
Code review ACK 75c3f9f880. Nice cleanups! Just suggested changes since last review: keeping UniqueLock name and fixing a missed rename in a code comment
Tree-SHA512: ec261f6a444bdfe4f06e844b57b3606fdd9b2f842647cae15266d9729970d87585c808d482fbba0b31c33a4aa03527c36e282c92b28d9052711f75a7048c96f1
