💬 laanwj commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2094679960)
> I've implemented DNSSEC
That's neat!
i think it's still missing some part, resolving through Google's DNS (which has more verbose error messages than my ISP) gives:
```sh
$ dig x9.dnsseed.signet.bitcoin.achow101.com. @1.1.1.1
⋮
;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; EDE: 10 (RRSIGs Missing): (failed to verify signatures for x9.dnsseed.signet.bitcoin.achow101.com. opt-out proof)
⋮
```
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2094679960)
> I've implemented DNSSEC
That's neat!
i think it's still missing some part, resolving through Google's DNS (which has more verbose error messages than my ISP) gives:
```sh
$ dig x9.dnsseed.signet.bitcoin.achow101.com. @1.1.1.1
⋮
;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; EDE: 10 (RRSIGs Missing): (failed to verify signatures for x9.dnsseed.signet.bitcoin.achow101.com. opt-out proof)
⋮
```
💬 theStack commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2094700451)
Rebased on https://github.com/bitcoin/bitcoin/pull/29612, supporting the latest format with enhanced metadata (magic bytes, version, network magic, block height, block hash, coins count).
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2094700451)
Rebased on https://github.com/bitcoin/bitcoin/pull/29612, supporting the latest format with enhanced metadata (magic bytes, version, network magic, block height, block hash, coins count).
⚠️ laanwj opened an issue: "upstream: GUIX closure contains too much unnecessary stuff"
(https://github.com/bitcoin/bitcoin/issues/30042)
The minimum guix closure contains X11 libraries as well as LaTeX and associated fonts. They should be unnecessary for minimum set of packages needed to build. Especially on slower CPUs (eg current RISC-V cores, qemu) it results in a large slowdown, it's also bad from a "minimum trusted set" point of view.
They're pulled in by `guix-manual`, as well as sometimes, package-specific documentation. Unfortunately, documentation is not behind an optional flag at the moment so there's nothing we can
...
(https://github.com/bitcoin/bitcoin/issues/30042)
The minimum guix closure contains X11 libraries as well as LaTeX and associated fonts. They should be unnecessary for minimum set of packages needed to build. Especially on slower CPUs (eg current RISC-V cores, qemu) it results in a large slowdown, it's also bad from a "minimum trusted set" point of view.
They're pulled in by `guix-manual`, as well as sometimes, package-specific documentation. Unfortunately, documentation is not behind an optional flag at the moment so there's nothing we can
...
💬 0xB10C commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-2094743012)
I've played around a bit with this in https://github.com/0xB10C/find-non-standard-tx/pull/2.
I've seen output for the following reasons:
- `too-long-mempool-chain`: e.g.
- `exceeds descendant size limit for tx 9f95e53da39ece93ceba3cc78a443a1aef0fc656bf921bbd2aee7f6533f0604f [limit: 101000]`
- `exceeds ancestor size limit [limit: 101000]`
- `2256302dedb181f83bc51248215e51f680f77d07e11ad7e9e34c2e483ad10e7b [limit: 26]`
- `min relay fee not met`: e.g. `0 < 200`
Other reasons lik
...
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-2094743012)
I've played around a bit with this in https://github.com/0xB10C/find-non-standard-tx/pull/2.
I've seen output for the following reasons:
- `too-long-mempool-chain`: e.g.
- `exceeds descendant size limit for tx 9f95e53da39ece93ceba3cc78a443a1aef0fc656bf921bbd2aee7f6533f0604f [limit: 101000]`
- `exceeds ancestor size limit [limit: 101000]`
- `2256302dedb181f83bc51248215e51f680f77d07e11ad7e9e34c2e483ad10e7b [limit: 26]`
- `min relay fee not met`: e.g. `0 < 200`
Other reasons lik
...
💬 theStack commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1590272199)
> Could also continue to get all the UTXOs up front?
Indeed. I thought it was a nice side-benefit of the ephemeral wallet if the manual coin selection wouldn't be needed anymore, but keeping it seems better than the alternatives. Thanks, done.
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1590272199)
> Could also continue to get all the UTXOs up front?
Indeed. I thought it was a nice side-benefit of the ephemeral wallet if the manual coin selection wouldn't be needed anymore, but keeping it seems better than the alternatives. Thanks, done.
💬 theStack commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1590272281)
Good point. I'm planning a follow-up where these prerequisites (-datacarriersize=100000 and -maxmempool=5) are checked at run-time in `fill_mempool`, in order to give explicit error messages if `fill_mempool` users forget to set these parameters. Will include a commit with your suggestion to add comments in functional tests where this is missing (seems to be both in `rpc_packages.py` and `p2p_tx_download.py`).
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1590272281)
Good point. I'm planning a follow-up where these prerequisites (-datacarriersize=100000 and -maxmempool=5) are checked at run-time in `fill_mempool`, in order to give explicit error messages if `fill_mempool` users forget to set these parameters. Will include a commit with your suggestion to add comments in functional tests where this is missing (seems to be both in `rpc_packages.py` and `p2p_tx_download.py`).
💬 theStack commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#issuecomment-2094748534)
Rebased on master (necessary since #28970 introduced new tests with `fill_mempool` call-sites) and addressed review comment https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1584441713.
(https://github.com/bitcoin/bitcoin/pull/29939#issuecomment-2094748534)
Rebased on master (necessary since #28970 introduced new tests with `fill_mempool` call-sites) and addressed review comment https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1584441713.
💬 0xB10C commented on pull request "tracing: explicitly cast block_connected duration to nanoseconds":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2094762513)
> > Any reason not to just stick to nanoseconds and harden it against future accidents?
>
> I think updating the documentation and using `Ticks<std::chrono::nanoseconds>` would work just as well, yes..
Done. For me, not breaking the (already broken) API again is easier. Especially when running current and future releases in parallel. Sorry to invalidate your ACK @maflcko @laanwj.
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2094762513)
> > Any reason not to just stick to nanoseconds and harden it against future accidents?
>
> I think updating the documentation and using `Ticks<std::chrono::nanoseconds>` would work just as well, yes..
Done. For me, not breaking the (already broken) API again is easier. Especially when running current and future releases in parallel. Sorry to invalidate your ACK @maflcko @laanwj.
💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2094796800)
The genesis block message is just for fun, since almost nobody will ever see it. There were half a dozen articles written about testnet recently, however the CCN article was the only one with a relevant title that mentioned resetting testnet. I don't think the "strength" of the message to prevent pre-mining really matters if we agree that the primary goal is for testnet tokens to not have value.
The goal of my griefing was to bring attention to this issue, and I dare say that it worked. We're
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2094796800)
The genesis block message is just for fun, since almost nobody will ever see it. There were half a dozen articles written about testnet recently, however the CCN article was the only one with a relevant title that mentioned resetting testnet. I don't think the "strength" of the message to prevent pre-mining really matters if we agree that the primary goal is for testnet tokens to not have value.
The goal of my griefing was to bring attention to this issue, and I dare say that it worked. We're
...
💬 vostrnad commented on issue "Possible to Ban Clients by Name?":
(https://github.com/bitcoin/bitcoin/issues/30036#issuecomment-2094814430)
Are you sure it's "Satoshi-BTC" and not "Satoshi-BTF"? bitnodes.io reports 12 nodes with the user agent [/Satoshi-BTF(BitcoinFinance):0.15.1/](https://bitnodes.io/nodes/?q=Satoshi-BTF%28BitcoinFinance%29%3A0.15.1), most of them in China.
(https://github.com/bitcoin/bitcoin/issues/30036#issuecomment-2094814430)
Are you sure it's "Satoshi-BTC" and not "Satoshi-BTF"? bitnodes.io reports 12 nodes with the user agent [/Satoshi-BTF(BitcoinFinance):0.15.1/](https://bitnodes.io/nodes/?q=Satoshi-BTF%28BitcoinFinance%29%3A0.15.1), most of them in China.
👋 theStack's pull request is ready for review: "contrib: add tool to convert compact-serialized UTXO set to SQLite database"
(https://github.com/bitcoin/bitcoin/pull/27432)
(https://github.com/bitcoin/bitcoin/pull/27432)
🤔 furszy reviewed a pull request: "rpc, wallet: fix incorrect segwit redeem script size limit"
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2039716714)
> I wrote a test for RPC signrawtransactionwithkey that covers legacy P2SH with 15 and 16 public keys. Both calls succeed even though the latter produces a scriptSig that exceeds MAX_STANDARD_SCRIPTSIG_SIZE and if it ever made it into a block, would exceed MAX_SCRIPT_ELEMENT_SIZE
The second call, the one with 16 pubkeys, doesn't entirely succeed. The test isn't checking the error field in the `signrawtransactionwithkey` response, which returns an `'error': 'Push value size limit exceeded'`.
...
(https://github.com/bitcoin/bitcoin/pull/28307#pullrequestreview-2039716714)
> I wrote a test for RPC signrawtransactionwithkey that covers legacy P2SH with 15 and 16 public keys. Both calls succeed even though the latter produces a scriptSig that exceeds MAX_STANDARD_SCRIPTSIG_SIZE and if it ever made it into a block, would exceed MAX_SCRIPT_ELEMENT_SIZE
The second call, the one with 16 pubkeys, doesn't entirely succeed. The test isn't checking the error field in the `signrawtransactionwithkey` response, which returns an `'error': 'Push value size limit exceeded'`.
...
💬 tdb3 commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2094859146)
Re ACK for d53d84834747c37f4060a9ef379e0a6b50155246
Pulled, built, ran all unit/functional tests (all passed).
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2094859146)
Re ACK for d53d84834747c37f4060a9ef379e0a6b50155246
Pulled, built, ran all unit/functional tests (all passed).
💬 Ferrydh commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2094860396)
这是来自QQ邮箱的假期自动回复邮件。
您好，我最近正在休假中，无法亲自回复您的邮件。我将在假期结束后，尽快给您回复。
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-2094860396)
这是来自QQ邮箱的假期自动回复邮件。
您好，我最近正在休假中，无法亲自回复您的邮件。我将在假期结束后，尽快给您回复。
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2094886890)
`d98ca056a8...bee22409ea`: rebase and address suggestions
(https://github.com/bitcoin/bitcoin/pull/29798#issuecomment-2094886890)
`d98ca056a8...bee22409ea`: rebase and address suggestions
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590369829)
Dropped the commit entirely as suggested.
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590369829)
Dropped the commit entirely as suggested.
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590369909)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590369909)
Done as suggested.
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590370012)
Done.
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590370012)
Done.
💬 achow101 commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2094889653)
> i think it's still missing some part, resolving through Google's DNS (which has more verbose error messages than my ISP) gives:
Should be fixed now
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2094889653)
> i think it's still missing some part, resolving through Google's DNS (which has more verbose error messages than my ISP) gives:
Should be fixed now
💬 vasild commented on pull request "Logging cleanup":
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590371367)
Reworded the commit message as suggested, but I didn't get this:
> Right now this returns NONE/true when the empty string "" is passed.
Hmm, no? It returns ALL/true when "" is passed, in `master` and in this PR - there is an `if (str.empty() ...` a few lines above.
> Should return false in that case too
You mean to return false when empty string is passed? That would be some further change that maybe makes sense, but is not included in this PR.
> and never return NONE?
It never
...
(https://github.com/bitcoin/bitcoin/pull/29798#discussion_r1590371367)
Reworded the commit message as suggested, but I didn't get this:
> Right now this returns NONE/true when the empty string "" is passed.
Hmm, no? It returns ALL/true when "" is passed, in `master` and in this PR - there is an `if (str.empty() ...` a few lines above.
> Should return false in that case too
You mean to return false when empty string is passed? That would be some further change that maybe makes sense, but is not included in this PR.
> and never return NONE?
It never
...