💬 achow101 commented on pull request "sqlite: Disallow writing from multiple `SQLiteBatch`s":
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1892439363)
> The db txn is still active at the engine level, the lock shouldn't be released when the sqlite statement fails to execute (only when the abort/commit command executes successfully). Otherwise, other concurrent handlers are allowed to access db at the same time and, in the worst possible outcome, dump information to disk that should have been discarded.
If it fails to abort when the batch is destructed, then we're deadlocked. I don't see how this can be resolved without deadlocking and still
...
(https://github.com/bitcoin/bitcoin/pull/29112#issuecomment-1892439363)
> The db txn is still active at the engine level, the lock shouldn't be released when the sqlite statement fails to execute (only when the abort/commit command executes successfully). Otherwise, other concurrent handlers are allowed to access db at the same time and, in the worst possible outcome, dump information to disk that should have been discarded.
If it fails to abort when the batch is destructed, then we're deadlocked. I don't see how this can be resolved without deadlocking and still
...
✅ fanquake closed a pull request: "doc: update encryptwallet passphrase doc"
(https://github.com/bitcoin/bitcoin/pull/29245)
(https://github.com/bitcoin/bitcoin/pull/29245)
💬 fanquake commented on pull request "doc: update encryptwallet passphrase doc":
(https://github.com/bitcoin/bitcoin/pull/29245#issuecomment-1892449257)
> Follow up to https://github.com/bitcoin/bitcoin/pull/28974, coming from https://github.com/bitcoin/bitcoin/pull/28974#issuecomment-1855661692
Given that #28974 has not been merged, you can just make any further changes, have further discussion in that PR. Not sure about adding all this text. In any case, you'll need to at least remove the markdown formatting.
(https://github.com/bitcoin/bitcoin/pull/29245#issuecomment-1892449257)
> Follow up to https://github.com/bitcoin/bitcoin/pull/28974, coming from https://github.com/bitcoin/bitcoin/pull/28974#issuecomment-1855661692
Given that #28974 has not been merged, you can just make any further changes, have further discussion in that PR. Not sure about adding all this text. In any case, you'll need to at least remove the markdown formatting.
📝 fanquake converted_to_draft a pull request: "Change Luke Dashjr seed to dashjr-list-of-p2p-nodes-maybe-malware.us"
(https://github.com/bitcoin/bitcoin/pull/29145)
To avoid issues with DNS blacklisting, I've setup a separate domain for my DNS seed.
Like #28936
I've chosen a domain name that is explicitly verbose about its purpose and the possibility of malware on resolved IPs, to go an extra mile in helping avoid any attempts to abuse it.
(https://github.com/bitcoin/bitcoin/pull/29145)
To avoid issues with DNS blacklisting, I've setup a separate domain for my DNS seed.
Like #28936
I've chosen a domain name that is explicitly verbose about its purpose and the possibility of malware on resolved IPs, to go an extra mile in helping avoid any attempts to abuse it.
💬 fanquake commented on pull request "Change Luke Dashjr seed to dashjr-list-of-p2p-nodes-maybe-malware.us":
(https://github.com/bitcoin/bitcoin/pull/29145#issuecomment-1892470188)
> but if there's a hard objection to it, I can come up with another domain for it.
I'd say objections to your current choice of domain name have been made clear from various contributors. Along with objection to changing any logging to accomodate it. Moved this to draft for now. Feel-free to undraft with a different domain etc.
(https://github.com/bitcoin/bitcoin/pull/29145#issuecomment-1892470188)
> but if there's a hard objection to it, I can come up with another domain for it.
I'd say objections to your current choice of domain name have been made clear from various contributors. Along with objection to changing any logging to accomodate it. Moved this to draft for now. Feel-free to undraft with a different domain etc.
📝 TheCharlatan opened a pull request: "contrib: Add --binary option to clang-format-diff"
(https://github.com/bitcoin/bitcoin/pull/29251)
This was adapted from https://github.com/llvm-mirror/clang/commit/145510a469c6efd18bd98cd3c3f306492a9af90d and is useful for systems where clang tools are shipped with a version suffix.
(https://github.com/bitcoin/bitcoin/pull/29251)
This was adapted from https://github.com/llvm-mirror/clang/commit/145510a469c6efd18bd98cd3c3f306492a9af90d and is useful for systems where clang tools are shipped with a version suffix.
📝 TheCharlatan opened a pull request: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252)
The key module's functionality is not used by the kernel library, but currently kernel users are still required to initialize the key module's `secp256k1_context_sign` global as part of the `kernel::Context` through `ECC_Start`. So move the `ECC_Start` call to the `NodeContext` ctor instead to completely remove the key module from the kernel library.
The gui tests currently keep multiple `NodeContext` objects in memory, so relax the `secp256k1_context_sign` check in `ECC_Start` to instead ret
...
(https://github.com/bitcoin/bitcoin/pull/29252)
The key module's functionality is not used by the kernel library, but currently kernel users are still required to initialize the key module's `secp256k1_context_sign` global as part of the `kernel::Context` through `ECC_Start`. So move the `ECC_Start` call to the `NodeContext` ctor instead to completely remove the key module from the kernel library.
The gui tests currently keep multiple `NodeContext` objects in memory, so relax the `secp256k1_context_sign` check in `ECC_Start` to instead ret
...
💬 DanGould commented on issue "Implement PayJoin / Pay-to-EndPoint":
(https://github.com/bitcoin/bitcoin/issues/19148#issuecomment-1892630259)
The [BIP-77: Payjoin V2](https://github.com/bitcoin/bips/pull/1483) proposal gets rid of the requirement for both parties to be online and the requirement to use TLS. It's authenticated with [Hybrid Public Key Encryption](https://datatracker.ietf.org/doc/rfc9180/) that depends on cryptographic primitives already in bitcoin instead (ChaCha20-Poly1305, Secp256k1, Sha256).
(https://github.com/bitcoin/bitcoin/issues/19148#issuecomment-1892630259)
The [BIP-77: Payjoin V2](https://github.com/bitcoin/bips/pull/1483) proposal gets rid of the requirement for both parties to be online and the requirement to use TLS. It's authenticated with [Hybrid Public Key Encryption](https://datatracker.ietf.org/doc/rfc9180/) that depends on cryptographic primitives already in bitcoin instead (ChaCha20-Poly1305, Secp256k1, Sha256).
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1892642385)
Added a (ephemeral) client id to the log so it's easier to figure out what's going on when there's multiple connections.
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1892642385)
Added a (ephemeral) client id to the log so it's easier to figure out what's going on when there's multiple connections.
💬 Kenshiro-28 commented on issue "Implement PayJoin / Pay-to-EndPoint":
(https://github.com/bitcoin/bitcoin/issues/19148#issuecomment-1892671561)
Great, I hope it's implemented :)
(https://github.com/bitcoin/bitcoin/issues/19148#issuecomment-1892671561)
Great, I hope it's implemented :)
🤔 furszy reviewed a pull request: "init: handle empty settings file gracefully"
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1822184082)
> But It does seem unfortunate that both of the commits here make tests more verbose and repetitive. And the tests even before this PR were probably unnecessarily fragile. But I'm not sure I see a better way to write the tests, or a way to avoid the repetition of warning and error messages. If anybody has any ideas on how to improve the tests here, I'd be interested to know.
This is not a small change, but.. we could work on a `Resources` class that is auto-generated during the compile phase.
...
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1822184082)
> But It does seem unfortunate that both of the commits here make tests more verbose and repetitive. And the tests even before this PR were probably unnecessarily fragile. But I'm not sure I see a better way to write the tests, or a way to avoid the repetition of warning and error messages. If anybody has any ideas on how to improve the tests here, I'd be interested to know.
This is not a small change, but.. we could work on a `Resources` class that is auto-generated during the compile phase.
...
🤔 stickies-v reviewed a pull request: "[25.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/28768#pullrequestreview-1822192937)
LGTM 115e31b8e902c02ad4b342843c4811b5e74415d0 modulo small release notes issue. Also, PR description needs to be updated to remove the reference to https://github.com/bitcoin/bitcoin/pull/28919.
(https://github.com/bitcoin/bitcoin/pull/28768#pullrequestreview-1822192937)
LGTM 115e31b8e902c02ad4b342843c4811b5e74415d0 modulo small release notes issue. Also, PR description needs to be updated to remove the reference to https://github.com/bitcoin/bitcoin/pull/28919.
💬 stickies-v commented on pull request "[25.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/28768#discussion_r1452702149)
I don't think this is backported?
(https://github.com/bitcoin/bitcoin/pull/28768#discussion_r1452702149)
I don't think this is backported?
💬 furszy commented on pull request "init: handle empty settings file gracefully":
(https://github.com/bitcoin/bitcoin/pull/29144#discussion_r1452708666)
Since the confusion over the empty settings file error is resolved by the first commit. I agree with ryanofsky; while extending the file warning message sounds plausible, I'm also ok with the current warning.
(https://github.com/bitcoin/bitcoin/pull/29144#discussion_r1452708666)
Since the confusion over the empty settings file error is resolved by the first commit. I agree with ryanofsky; while extending the file warning message sounds plausible, I'm also ok with the current warning.
👍 TheCharlatan approved a pull request: "contrib: add macho branch protection check"
(https://github.com/bitcoin/bitcoin/pull/29170#pullrequestreview-1822235032)
ACK 5335e454c0889c8a1bb05aa09435883322133974
(https://github.com/bitcoin/bitcoin/pull/29170#pullrequestreview-1822235032)
ACK 5335e454c0889c8a1bb05aa09435883322133974
💬 mzumsande commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1452746812)
I think that followed from the `@return` doc, but it wasn't so obvious that adding an existing addr to another new bucket also affects the return value; Changed the `@return` description to `true if at least one address is successfully added, or added to an additional bucket. Unaffected by updates.`
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1452746812)
I think that followed from the `@return` doc, but it wasn't so obvious that adding an existing addr to another new bucket also affects the return value; Changed the `@return` description to `true if at least one address is successfully added, or added to an additional bucket. Unaffected by updates.`
💬 mzumsande commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1452746873)
done
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1452746873)
done
💬 mzumsande commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1452747951)
I think it's better if the header doesn't refer to implementation details. If I remember correctly, this logic used to be in `Add()`, then `Add_()`, then `AddSingle()`, and I don't think it's nice if we have to update `addrman.h` for refactors like that.
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1452747951)
I think it's better if the header doesn't refer to implementation details. If I remember correctly, this logic used to be in `Add()`, then `Add_()`, then `AddSingle()`, and I don't think it's nice if we have to update `addrman.h` for refactors like that.
💬 mzumsande commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1452749219)
similar to above, I don't like referring to implementation details in comments unless really necessary.
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1452749219)
similar to above, I don't like referring to implementation details in comments unless really necessary.
💬 mzumsande commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1892777051)
[d536e5a ](https://github.com/bitcoin/bitcoin/commit/d536e5a6325d1885224f36debdcc5245b94efe8a)to [74ebd4d](https://github.com/bitcoin/bitcoin/commit/74ebd4d1359edce82a134dfcd3da9840f8d206e2): Addressed feedback
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1892777051)
[d536e5a ](https://github.com/bitcoin/bitcoin/commit/d536e5a6325d1885224f36debdcc5245b94efe8a)to [74ebd4d](https://github.com/bitcoin/bitcoin/commit/74ebd4d1359edce82a134dfcd3da9840f8d206e2): Addressed feedback