📝 ryanofsky opened a pull request: "Improve new LogDebug/Trace/Info/Warning/Error Macros"
(https://github.com/bitcoin/bitcoin/pull/29256)
Improve new LogDebug(), LogTrace(), LogInfo(), LogWarning(), LogError() macros introduced in #28318:
- Make them always accept log categories to make it possible to only log messages from a particular component.
- Make them compatible with wallet logging, which includes the wallet name in log messages
- Make them not rely on a global LogInstance, to provide better control of logging in controlled environments, such as unit tests that want to selectively capture log output, or libbitcoinker
...
(https://github.com/bitcoin/bitcoin/pull/29256)
Improve new LogDebug(), LogTrace(), LogInfo(), LogWarning(), LogError() macros introduced in #28318:
- Make them always accept log categories to make it possible to only log messages from a particular component.
- Make them compatible with wallet logging, which includes the wallet name in log messages
- Make them not rely on a global LogInstance, to provide better control of logging in controlled environments, such as unit tests that want to selectively capture log output, or libbitcoinker
...
💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-1894600399)
Updated 2cb38d93a8fea16bf84b072a90ac023ef345134d -> a885a166cec6d84d08600f12b25d912bd28af80e ([kernelRmKey_1](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_1) -> [kernelRmKey_2](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmKey_1..kernelRmKey_2))
* Addressed @maflcko's [comment](https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453627000), re-added explicit context destructor.
* Addressed @ma
...
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-1894600399)
Updated 2cb38d93a8fea16bf84b072a90ac023ef345134d -> a885a166cec6d84d08600f12b25d912bd28af80e ([kernelRmKey_1](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_1) -> [kernelRmKey_2](https://github.com/TheCharlatan/bitcoin/tree/kernelRmKey_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelRmKey_1..kernelRmKey_2))
* Addressed @maflcko's [comment](https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1453627000), re-added explicit context destructor.
* Addressed @ma
...
💬 ryanofsky commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1454120255)
> Will see how this looks and probably open a PR
Opened #29256 with these changes. Code changes should be complete but there are some test failures so it is a draft.
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1454120255)
> Will see how this looks and probably open a PR
Opened #29256 with these changes. Code changes should be complete but there are some test failures so it is a draft.
✅ aureleoules closed a pull request: "refactor(tidy): Use C++20 contains method"
(https://github.com/bitcoin/bitcoin/pull/29191)
(https://github.com/bitcoin/bitcoin/pull/29191)
💬 aureleoules commented on pull request "refactor(tidy): Use C++20 contains method":
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1894612555)
> FWIW I would approve this PR if it would only contain the changes that are automatically picked up by tidy and applied by running with -fix, making resolving potential conflicts purely manual.
Sure, I agree. Closing this pull.
(https://github.com/bitcoin/bitcoin/pull/29191#issuecomment-1894612555)
> FWIW I would approve this PR if it would only contain the changes that are automatically picked up by tidy and applied by running with -fix, making resolving potential conflicts purely manual.
Sure, I agree. Closing this pull.
💬 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
...