💬 ajtowns commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1894630511)
Concept NACK from me, this seems much uglier and trickier to work with. I've already written [in depth](https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1702318968) about why the current approach makes sense, so I'll be brief here.
> Make them always accept log categories to make it possible to only log messages from a particular component.
Being able to avoid logging critical messages is adding a bug, not a feature.
> Make them less verbose by not requiring BCLog category con
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1894630511)
Concept NACK from me, this seems much uglier and trickier to work with. I've already written [in depth](https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1702318968) about why the current approach makes sense, so I'll be brief here.
> Make them always accept log categories to make it possible to only log messages from a particular component.
Being able to avoid logging critical messages is adding a bug, not a feature.
> Make them less verbose by not requiring BCLog category con
...
💬 chrisguida commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1894632119)
Concept ACK, what will it take to get this merged? Stamps are absolutely ravaging the UTXO set.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1894632119)
Concept ACK, what will it take to get this merged? Stamps are absolutely ravaging the UTXO set.
🤔 mzumsande reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1825332930)
If I understand it correctly, the logic deciding whether to try and accept outbound connections, using `HasAllDesirableServiceFlags()`, relies on `IsInitialBlockDownload()` / `DEFAULT_MAX_TIP_AGE` (24h) on master, but is changed to rely on `NODE_NETWORK_LIMITED_MIN_BLOCKS` (244 blocks, ~48h) in this PR.
I think this means that the safety buffer of 144 blocks proposed in [BIP159](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki) would have been removed. I'm not sure about that, t
...
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1825332930)
If I understand it correctly, the logic deciding whether to try and accept outbound connections, using `HasAllDesirableServiceFlags()`, relies on `IsInitialBlockDownload()` / `DEFAULT_MAX_TIP_AGE` (24h) on master, but is changed to rely on `NODE_NETWORK_LIMITED_MIN_BLOCKS` (244 blocks, ~48h) in this PR.
I think this means that the safety buffer of 144 blocks proposed in [BIP159](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki) would have been removed. I'm not sure about that, t
...
💬 mzumsande commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1454112543)
I don't think that `CheckForStaleTipAndEvictPeers` triggers any status updates with the current approach, only `UpdatedBlockTip` does. Maybe this is left from an earlier version? In any case, if I remove all calls to it, the test still succeeds.
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1454112543)
I don't think that `CheckForStaleTipAndEvictPeers` triggers any status updates with the current approach, only `UpdatedBlockTip` does. Maybe this is left from an earlier version? In any case, if I remove all calls to it, the test still succeeds.
⚠️ mrdomino opened an issue: "bitcoin.org still lists laanwj's now-revoked PGP key"
(https://github.com/bitcoin/bitcoin/issues/29257)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The instructions at https://bitcoin.org/en/full-node#linux-instructions still say:
> The 0.11 and later releases are signed by [Wladimir J. van der Laan’s releases key](https://bitcoin.org/laanwj-releases.asc) with the fingerprint:
>
> `01EA 5486 DE18 A882 D4C2 6845 90C8 019E 36C2 E964`
But the key expired, and was revoked on 2023-02-11, and is no longer used to sign `SHA256SUM.a
...
(https://github.com/bitcoin/bitcoin/issues/29257)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
The instructions at https://bitcoin.org/en/full-node#linux-instructions still say:
> The 0.11 and later releases are signed by [Wladimir J. van der Laan’s releases key](https://bitcoin.org/laanwj-releases.asc) with the fingerprint:
>
> `01EA 5486 DE18 A882 D4C2 6845 90C8 019E 36C2 E964`
But the key expired, and was revoked on 2023-02-11, and is no longer used to sign `SHA256SUM.a
...
✅ mrdomino closed an issue: "bitcoin.org still lists laanwj's now-revoked PGP key"
(https://github.com/bitcoin/bitcoin/issues/29257)
(https://github.com/bitcoin/bitcoin/issues/29257)
💬 mrdomino commented on issue "bitcoin.org still lists laanwj's now-revoked PGP key":
(https://github.com/bitcoin/bitcoin/issues/29257#issuecomment-1894791931)
I see now that you guys may not own bitcoin.org.
(https://github.com/bitcoin/bitcoin/issues/29257#issuecomment-1894791931)
I see now that you guys may not own bitcoin.org.
💬 ajtowns commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1894895307)
> Concept ACK, what will it take to get this merged? Stamps are absolutely ravaging the UTXO set.
Nobody's given anything more than a concept ack, would be unusual to merge without some actual acks. Also the code should be rebased onto master (`git rebase origin/master`), at present there's a merge commit, which would also be unusual to have merged.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1894895307)
> Concept ACK, what will it take to get this merged? Stamps are absolutely ravaging the UTXO set.
Nobody's given anything more than a concept ack, would be unusual to merge without some actual acks. Also the code should be rebased onto master (`git rebase origin/master`), at present there's a merge commit, which would also be unusual to have merged.
💬 luke-jr commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1894900263)
It needs a rebase, unless we're going to be okay with merge commits now (I'm fine with it, but historically it's been a blocker)
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1894900263)
It needs a rebase, unless we're going to be okay with merge commits now (I'm fine with it, but historically it's been a blocker)
💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1894928988)
Hi AJ, this is a draft, and there will be some more changes which may address your concerns.
> > Make them always accept log categories to make it possible to only log messages from a particular component.
> Being able to avoid logging critical messages is adding a bug, not a feature.
Agree, but the idea here is not to discard log messages, the idea just is to attach meaningful metadata to log messages so they can be filtered by component.
> > Make them less verbose by not requiring BC
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1894928988)
Hi AJ, this is a draft, and there will be some more changes which may address your concerns.
> > Make them always accept log categories to make it possible to only log messages from a particular component.
> Being able to avoid logging critical messages is adding a bug, not a feature.
Agree, but the idea here is not to discard log messages, the idea just is to attach meaningful metadata to log messages so they can be filtered by component.
> > Make them less verbose by not requiring BC
...
⚠️ ajtowns opened an issue: "Update nChainTx to 64bit type"
(https://github.com/bitcoin/bitcoin/issues/29258)
https://github.com/bitcoin/bitcoin/blob/8106b268cde8e97a7c330afdda39b6bb55e5574a/src/chain.h#L178-L186
Hey, it's 2024! `nChainTx` seems to currently be 953368191 (height 826100), which is hex `38d3 3e7f`, so we're only 22% of the way to an overflow, and would still need another 200k blocks (~ four years) full of 60 byte transactions to get there.
(https://github.com/bitcoin/bitcoin/issues/29258)
https://github.com/bitcoin/bitcoin/blob/8106b268cde8e97a7c330afdda39b6bb55e5574a/src/chain.h#L178-L186
Hey, it's 2024! `nChainTx` seems to currently be 953368191 (height 826100), which is hex `38d3 3e7f`, so we're only 22% of the way to an overflow, and would still need another 200k blocks (~ four years) full of 60 byte transactions to get there.
👍 shaavan approved a pull request: "refactor: Allow std::span construction from CKey"
(https://github.com/bitcoin/bitcoin/pull/29133#pullrequestreview-1826678191)
ReACK fa96d937116682f32613d31a3ae7d6f652e8146d
Changes since my last [review](https://github.com/bitcoin/bitcoin/pull/29133#pullrequestreview-1800426197)
- Removed the last commit, which was temporarily introduced to test the workings of changes in this commit.
- Tested that the above patch leads to failure in master, while successful compilation with this PR.
(https://github.com/bitcoin/bitcoin/pull/29133#pullrequestreview-1826678191)
ReACK fa96d937116682f32613d31a3ae7d6f652e8146d
Changes since my last [review](https://github.com/bitcoin/bitcoin/pull/29133#pullrequestreview-1800426197)
- Removed the last commit, which was temporarily introduced to test the workings of changes in this commit.
- Tested that the above patch leads to failure in master, while successful compilation with this PR.
💬 ajtowns commented on issue "Update nChainTx to 64bit type":
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1895271451)
Background: https://github.com/bitcoin/bitcoin/pull/13875 and https://github.com/bitcoin/bitcoin/pull/1677
(https://github.com/bitcoin/bitcoin/issues/29258#issuecomment-1895271451)
Background: https://github.com/bitcoin/bitcoin/pull/13875 and https://github.com/bitcoin/bitcoin/pull/1677
📝 MarnixCroes opened a pull request: "gui: debugwindow: update session ID tooltip"
(https://github.com/bitcoin-core/gui/pull/788)
When you have a v2 connection, there is always a session ID.
the _if any_ is a leftover from https://github.com/bitcoin-core/gui/pull/754, where the session ID property initially would always be displayed (transport v1 and v2).
So the session ID could be empty when you have a v1 connection.
As now the _Session ID_ property only is displayed for v2 connection, there will always be a session ID.
master
When you have a v2 connection, there is always a session ID.
the _if any_ is a leftover from https://github.com/bitcoin-core/gui/pull/754, where the session ID property initially would always be displayed (transport v1 and v2).
So the session ID could be empty when you have a v1 connection.
As now the _Session ID_ property only is displayed for v2 connection, there will always be a session ID.
master
![sessionIDifany](https://github.com/bitcoin-core/gui/assets/93143998/d4d7df43-828
...
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1895309927)
Thank you for the review @maflcko,
Updated d63b2e88780dc78fd531b053653361a0bf3fcbea -> 5755454fb5cc4067fc94e2682116e0fc5c9dfc58 ([noGlobalSignals_1](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_1) -> [noGlobalSignals_2](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_1..noGlobalSignals_2))
* Added a new first scripted-diff commit for renaming `MainSignals` to `ValidationSignals` as su
...
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1895309927)
Thank you for the review @maflcko,
Updated d63b2e88780dc78fd531b053653361a0bf3fcbea -> 5755454fb5cc4067fc94e2682116e0fc5c9dfc58 ([noGlobalSignals_1](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_1) -> [noGlobalSignals_2](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_1..noGlobalSignals_2))
* Added a new first scripted-diff commit for renaming `MainSignals` to `ValidationSignals` as su
...
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1454918683)
Ah, I think we need the full declaration here, because other modules are now instantiating the `CMainSignals`.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1454918683)
Ah, I think we need the full declaration here, because other modules are now instantiating the `CMainSignals`.
💬 TheCharlatan commented on pull request "contrib: Add -binary option to clang-format-diff":
(https://github.com/bitcoin/bitcoin/pull/29251#issuecomment-1895327695)
Updated c7cb63a12e1a90fe46471cca4001a472078b5ccf -> 008e81e025d64c33e1e71b5a2fe63dfdf6b31437 ([ClangFormatDiffBinaryOption_0](https://github.com/TheCharlatan/bitcoin/tree/ClangFormatDiffBinaryOption_0) -> [ClangFormatDiffBinaryOption_1](https://github.com/TheCharlatan/bitcoin/tree/ClangFormatDiffBinaryOption_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/ClangFormatDiffBinaryOption_0..ClangFormatDiffBinaryOption_1))
* Addressed @maflcko's [comment](https://github.com/bitcoin/bi
...
(https://github.com/bitcoin/bitcoin/pull/29251#issuecomment-1895327695)
Updated c7cb63a12e1a90fe46471cca4001a472078b5ccf -> 008e81e025d64c33e1e71b5a2fe63dfdf6b31437 ([ClangFormatDiffBinaryOption_0](https://github.com/TheCharlatan/bitcoin/tree/ClangFormatDiffBinaryOption_0) -> [ClangFormatDiffBinaryOption_1](https://github.com/TheCharlatan/bitcoin/tree/ClangFormatDiffBinaryOption_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/ClangFormatDiffBinaryOption_0..ClangFormatDiffBinaryOption_1))
* Addressed @maflcko's [comment](https://github.com/bitcoin/bi
...
💬 maflcko commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1895368323)
Which filesystem is the blocks directory on?
Otherwise, this may be due to overheating, which will only happen under CPU load, and not if a mere filesystem check tool is run.
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1895368323)
Which filesystem is the blocks directory on?
Otherwise, this may be due to overheating, which will only happen under CPU load, and not if a mere filesystem check tool is run.
💬 maflcko commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-1895372210)
lgtm ACK a885a166cec6d84d08600f12b25d912bd28af80e 🔀
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK a885a166cec6d84d
...
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-1895372210)
lgtm ACK a885a166cec6d84d08600f12b25d912bd28af80e 🔀
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK a885a166cec6d84d
...
💬 Xaspr commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1895417277)
The whole SSD is just 1 partition in NTFS. There are no other drives.
I'll look into overheating as well.
By the way, v24.2 crashed after about 4 hours. Now looking into trying v26.0.0 with antivirus software disabled.
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1895417277)
The whole SSD is just 1 partition in NTFS. There are no other drives.
I'll look into overheating as well.
By the way, v24.2 crashed after about 4 hours. Now looking into trying v26.0.0 with antivirus software disabled.