💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1895858869)
Updated per review, thanks @mzumsande!
Have reduced the limited peers connections window to 144 blocks (previously set to the limited peers threshold value) as it is encouraged by the [BIP159](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki). And cleaned previous implementation leftovers in the test. [Code diff](https://github.com/bitcoin/bitcoin/compare/79e10351b1ce1f8db98ece67aacc24f323008b37..83e77ce1de2d4c4739bba71e4e27a4690c577375).
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1895858869)
Updated per review, thanks @mzumsande!
Have reduced the limited peers connections window to 144 blocks (previously set to the limited peers threshold value) as it is encouraged by the [BIP159](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki). And cleaned previous implementation leftovers in the test. [Code diff](https://github.com/bitcoin/bitcoin/compare/79e10351b1ce1f8db98ece67aacc24f323008b37..83e77ce1de2d4c4739bba71e4e27a4690c577375).
💬 GregTonoski commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1895881089)
> what alternative encoding scheme(s) the inscribers can be expected to migrate to, on a cost minimization basis, and what implications that will have on things like full node resource usage, and whether we would be happy with that.
If it had been about cost minimisation only then they would have spammed testnets (for virtually free) instead of the Mainchain.
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1895881089)
> what alternative encoding scheme(s) the inscribers can be expected to migrate to, on a cost minimization basis, and what implications that will have on things like full node resource usage, and whether we would be happy with that.
If it had been about cost minimisation only then they would have spammed testnets (for virtually free) instead of the Mainchain.
💬 Sjors commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895881805)
> A Cirrus account is free for public open source projects, as well as persistent workers connected to it.
> The workers currently use Cirrus self-hosted ones, so no changes are needed. This is documented in the cirrus yaml
This is a bit confusing. It's free but we use self hosting instead? Is that for performance, security some configuration issue?
I just gave Cirrus permission to run on my fork, but it complains "No public persistent worker pools found!" for all jobs. So I guess self
...
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895881805)
> A Cirrus account is free for public open source projects, as well as persistent workers connected to it.
> The workers currently use Cirrus self-hosted ones, so no changes are needed. This is documented in the cirrus yaml
This is a bit confusing. It's free but we use self hosting instead? Is that for performance, security some configuration issue?
I just gave Cirrus permission to run on my fork, but it complains "No public persistent worker pools found!" for all jobs. So I guess self
...
💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1455658183)
CompareFeerateDiagram won't return much very interesting, but you're right for the `err_string`, will return it
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1455658183)
CompareFeerateDiagram won't return much very interesting, but you're right for the `err_string`, will return it
💬 tromp commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1895893583)
> If it had been about cost minimisation then they would have spammed testnets
Their perceived value is of inscriptions on the bitcoin *main* chain of course. Not on a testnet, not on an altcoin chain, and not of an inscribed link to some other hosting site. The want their data to still be downloadable a century from now, guaranteed. Any cost minimization is conditional on that.
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1895893583)
> If it had been about cost minimisation then they would have spammed testnets
Their perceived value is of inscriptions on the bitcoin *main* chain of course. Not on a testnet, not on an altcoin chain, and not of an inscribed link to some other hosting site. The want their data to still be downloadable a century from now, guaranteed. Any cost minimization is conditional on that.
💬 maflcko commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895893619)
The assumptions for the runners are documented where they are configured: https://github.com/bitcoin/bitcoin/blob/master/.cirrus.yml#L16
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895893619)
The assumptions for the runners are documented where they are configured: https://github.com/bitcoin/bitcoin/blob/master/.cirrus.yml#L16
💬 Sjors commented on pull request "Support self hosted Github workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895905890)
I'll try to get that to work. At first glance it doesn't seem more complicated than the Github approach. If so then I'll close this and salvage the first two commits.
(https://github.com/bitcoin/bitcoin/pull/29259#issuecomment-1895905890)
I'll try to get that to work. At first glance it doesn't seem more complicated than the Github approach. If so then I'll close this and salvage the first two commits.
💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455548750)
second commit: I still don't understand why sometimes a nullptr check is done and sometimes not
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455548750)
second commit: I still don't understand why sometimes a nullptr check is done and sometimes not
💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455680501)
What is the point of asserting here, when previously it wasn't (and already ran into UB)?
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455680501)
What is the point of asserting here, when previously it wasn't (and already ran into UB)?
🤔 maflcko reviewed a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1827284733)
The assertions still seem a bit arbitrary
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1827284733)
The assertions still seem a bit arbitrary
💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455681352)
Maybe assert in the first line of this function and create a reference?
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455681352)
Maybe assert in the first line of this function and create a reference?
💬 GregTonoski commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1895913847)
> and not of an inscribed link to some other hosting site
To the contrary, they put text (link) in many cases, e.g. txid: ddcec4687acb054e94f5ad803b1f87574e93a37e560b0011b76ed8f36d6ad88a
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1895913847)
> and not of an inscribed link to some other hosting site
To the contrary, they put text (link) in many cases, e.g. txid: ddcec4687acb054e94f5ad803b1f87574e93a37e560b0011b76ed8f36d6ad88a
💬 jamesob commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1895934159)
Oddly enough, this change benched out as being slightly slower (~4.8%) than the commit before it in master.
Not a huge regression, but not what I expected. Compiled with `gcc 10.2.1`.
### #29169 vs. f8ca1357 (absolute)
| bench name | x | #29169 | f8ca1357 |
| --------
...
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1895934159)
Oddly enough, this change benched out as being slightly slower (~4.8%) than the commit before it in master.
Not a huge regression, but not what I expected. Compiled with `gcc 10.2.1`.
### #29169 vs. f8ca1357 (absolute)
| bench name | x | #29169 | f8ca1357 |
| --------
...
⚠️ maflcko opened an issue: "assumeutxo: nTx and nChainTx violations in CheckBlockIndex"
(https://github.com/bitcoin/bitcoin/issues/29261)
When disabling the "test-only" assumptions in CheckBlockIndex, the check fails. This is problematic, because test-only code should not affect the behavior of the program in production.
Current diff:
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 8c583c586c..00d7ee3736 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4866,9 +4866,9 @@ void ChainstateManager::CheckBlockIndex()
unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx
...
(https://github.com/bitcoin/bitcoin/issues/29261)
When disabling the "test-only" assumptions in CheckBlockIndex, the check fails. This is problematic, because test-only code should not affect the behavior of the program in production.
Current diff:
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 8c583c586c..00d7ee3736 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4866,9 +4866,9 @@ void ChainstateManager::CheckBlockIndex()
unsigned int prev_chain_tx = pindex->pprev ? pindex->pprev->nChainTx
...
💬 maflcko commented on pull request "AssumeUTXO follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28562#discussion_r1455739230)
Turned into an issue: https://github.com/bitcoin/bitcoin/issues/29261
(https://github.com/bitcoin/bitcoin/pull/28562#discussion_r1455739230)
Turned into an issue: https://github.com/bitcoin/bitcoin/issues/29261
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455749485)
I'm also not sure what to do with this one. We seem to be fine with not doing nullptr checks for very similar code, e.g. `base.cpp:89`, so I did not include any checks here. Maybe it is better to add a reference to the signals to `BaseIndex`?
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455749485)
I'm also not sure what to do with this one. We seem to be fine with not doing nullptr checks for very similar code, e.g. `base.cpp:89`, so I did not include any checks here. Maybe it is better to add a reference to the signals to `BaseIndex`?
💬 maflcko commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455760981)
> We seem to be fine with not doing nullptr checks for very similar code, e.g. base.cpp:89
That isn't run during shutdown, but during start, so the code path should be exercised in tests. Also, it is only "temporary", so will be removed in any case. Whereas the code here is run during shutdown, for which it is hard (impossible?) to test all code path permutations. Also it isn't temporary, is it?
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455760981)
> We seem to be fine with not doing nullptr checks for very similar code, e.g. base.cpp:89
That isn't run during shutdown, but during start, so the code path should be exercised in tests. Also, it is only "temporary", so will be removed in any case. Whereas the code here is run during shutdown, for which it is hard (impossible?) to test all code path permutations. Also it isn't temporary, is it?
👍 willcl-ark approved a pull request: "refactor: Allow std::span construction from CKey"
(https://github.com/bitcoin/bitcoin/pull/29133#pullrequestreview-1827516229)
ACK fa96d937116682f32613d31a3ae7d6f652e8146d
A nice simple change to enable using `std::span` with `CKey`.
The PR description could be updated now that the verification commit has been removed.
(https://github.com/bitcoin/bitcoin/pull/29133#pullrequestreview-1827516229)
ACK fa96d937116682f32613d31a3ae7d6f652e8146d
A nice simple change to enable using `std::span` with `CKey`.
The PR description could be updated now that the verification commit has been removed.
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455808582)
Right, not temporary, and in the `Stop` case we should guard against early destruction of the signals in the `Shutdown` procedure. I'll add an `Assert` for this case.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1455808582)
Right, not temporary, and in the `Stop` case we should guard against early destruction of the signals in the `Shutdown` procedure. I'll add an `Assert` for this case.
🤔 mzumsande reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1827576365)
> Hmm, good point. I would slightly rephrase, and extend, your first paragraph to state that the net limited buffer is connected to a user-customizable parameter, `-maxtipage`, with its default value is set to 24h.
Yes, but `-maxtipage` is just an undocumented debug-only option meant for testing, nothing to be used by actual users on mainnet.
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1827576365)
> Hmm, good point. I would slightly rephrase, and extend, your first paragraph to state that the net limited buffer is connected to a user-customizable parameter, `-maxtipage`, with its default value is set to 24h.
Yes, but `-maxtipage` is just an undocumented debug-only option meant for testing, nothing to be used by actual users on mainnet.