💬 furszy commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1892185801)
Thanks for the review @andrewtoth!
All nits tackled, [small diff](https://github.com/bitcoin/bitcoin/compare/6c603490023317a84c7c96ef4b64f4f96ea03d1f..79e10351b1ce1f8db98ece67aacc24f323008b37). Ready to go.
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1892185801)
Thanks for the review @andrewtoth!
All nits tackled, [small diff](https://github.com/bitcoin/bitcoin/compare/6c603490023317a84c7c96ef4b64f4f96ea03d1f..79e10351b1ce1f8db98ece67aacc24f323008b37). Ready to go.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1892249295)
Rebased on fix for CI issue
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1892249295)
Rebased on fix for CI issue
⚠️ Laughter79 opened an issue: "https://api.substack.com/feed/podcast/375278/private/b6bb76fe-b98c-4862-8885-1b0beb0fb3e4.rss"
(https://github.com/bitcoin/bitcoin/issues/29250)
(https://github.com/bitcoin/bitcoin/issues/29250)
:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/29250)
(https://github.com/bitcoin/bitcoin/issues/29250)
💬 fanquake commented on pull request "build: remove `--enable-lto`":
(https://github.com/bitcoin/bitcoin/pull/29185#issuecomment-1892283371)
This is waiting on https://github.com/google/oss-fuzz/pull/11498 downstream.
(https://github.com/bitcoin/bitcoin/pull/29185#issuecomment-1892283371)
This is waiting on https://github.com/google/oss-fuzz/pull/11498 downstream.
🤔 furszy reviewed a pull request: "sqlite: Disallow writing from multiple `SQLiteBatch`s"
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1821862931)
> Changed TxnAbort and TxnCommit to always release the lock.
This breaks the db txn isolation property.
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.
Check this out 71170998. Made a te
...
(https://github.com/bitcoin/bitcoin/pull/29112#pullrequestreview-1821862931)
> Changed TxnAbort and TxnCommit to always release the lock.
This breaks the db txn isolation property.
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.
Check this out 71170998. Made a te
...
💬 maflcko commented on pull request "ci, iwyu: Update mappings":
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1452493222)
are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/27710#discussion_r1452493222)
are you still working on this?
🚀 fanquake merged a pull request: "log mempool loading progress"
(https://github.com/bitcoin/bitcoin/pull/29227)
(https://github.com/bitcoin/bitcoin/pull/29227)
💬 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.
...