📝 romanz opened a pull request: "doc/zmq: fix unix socket path example"
(https://github.com/bitcoin/bitcoin/pull/33070)
Following https://github.com/bitcoin/bitcoin/blob/75a5c8258ec5309fe506438aa3815608430b53d6/doc/release-notes/release-notes-28.0.md?plain=1#L105
(https://github.com/bitcoin/bitcoin/pull/33070)
Following https://github.com/bitcoin/bitcoin/blob/75a5c8258ec5309fe506438aa3815608430b53d6/doc/release-notes/release-notes-28.0.md?plain=1#L105
👍 pinheadmz approved a pull request: "doc/zmq: fix unix socket path example"
(https://github.com/bitcoin/bitcoin/pull/33070#pullrequestreview-3058009745)
tested ACK
following existing directions:
`Error: Invalid port specified in -zmqpubrawtx: 'ipc:///tmp/bitcoind.tx.raw'`
(https://github.com/bitcoin/bitcoin/pull/33070#pullrequestreview-3058009745)
tested ACK
following existing directions:
`Error: Invalid port specified in -zmqpubrawtx: 'ipc:///tmp/bitcoind.tx.raw'`
💬 kristapsk commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-3121742667)
Rebased
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-3121742667)
Rebased
🤔 cedwies reviewed a pull request: "doc/zmq: fix unix socket path example"
(https://github.com/bitcoin/bitcoin/pull/33070#pullrequestreview-3058044842)
Ran bitcoind with both example endpoints.
ipc:///tmp/bitcoind.tx.raw: exits with "invalid port specified"
unix:/tmp/bitcoind.tx.raw: node starts cleanly
interface_zmq.py passed
ACK e83699a
(https://github.com/bitcoin/bitcoin/pull/33070#pullrequestreview-3058044842)
Ran bitcoind with both example endpoints.
ipc:///tmp/bitcoind.tx.raw: exits with "invalid port specified"
unix:/tmp/bitcoind.tx.raw: node starts cleanly
interface_zmq.py passed
ACK e83699a
💬 GregTonoski commented on issue "bitcoind shouldn't fail to progress with synchronization: endless [leveldb] Generated table ... logs":
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-3122036461)
I'm sharing logs of the second test (started on Jul 7 15:23:39).
[20250707logs.txt](https://github.com/user-attachments/files/21445305/20250707logs.txt)
[debug.log](https://github.com/user-attachments/files/21445306/debug.log)
[du.txt](https://github.com/user-attachments/files/21445307/du.txt)
I will provide my interpretation later.
(https://github.com/bitcoin/bitcoin/issues/31882#issuecomment-3122036461)
I'm sharing logs of the second test (started on Jul 7 15:23:39).
[20250707logs.txt](https://github.com/user-attachments/files/21445305/20250707logs.txt)
[debug.log](https://github.com/user-attachments/files/21445306/debug.log)
[du.txt](https://github.com/user-attachments/files/21445307/du.txt)
I will provide my interpretation later.
💬 yancyribbens commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233067368)
Well since I see you changed the assertion, then I think your rewording also makes sense.
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233067368)
Well since I see you changed the assertion, then I think your rewording also makes sense.
💬 yancyribbens commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3122102430)
> Yeah, as you noted, Single Random Draw is non-deterministic, so you would only get a predictable input set if there is only exactly one composition permitted. I don’t think that’s necessary to test what you want, though:
Well we all know the algorithm is non-deterministic, however ideally it would be possible to make deterministic test cases :).
I see you slightly weakened the assertion to handle the non-determinism which is a definite improvement over the current test.
(https://github.com/bitcoin/bitcoin/pull/33058#issuecomment-3122102430)
> Yeah, as you noted, Single Random Draw is non-deterministic, so you would only get a predictable input set if there is only exactly one composition permitted. I don’t think that’s necessary to test what you want, though:
Well we all know the algorithm is non-deterministic, however ideally it would be possible to make deterministic test cases :).
I see you slightly weakened the assertion to handle the non-determinism which is a definite improvement over the current test.
💬 yancyribbens commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233069431)
This doesn't show the behavior of the re-ordering done by the min-heap but it's better than before. And I think it would be a lot of work to make this test deterministic.
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233069431)
This doesn't show the behavior of the re-ordering done by the min-heap but it's better than before. And I think it would be a lot of work to make this test deterministic.
💬 yancyribbens commented on pull request "test: add assertions to SRD max weight test":
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233084856)
I removed the check for res since it's implied by the second assertion.
(https://github.com/bitcoin/bitcoin/pull/33058#discussion_r2233084856)
I removed the check for res since it's implied by the second assertion.
💬 nervana21 commented on pull request "rpc, wallet: replace remaining hardcoded output types with `FormatAllOutputTypes`":
(https://github.com/bitcoin/bitcoin/pull/33065#issuecomment-3122169008)
tACK [251d020](https://github.com/bitcoin/bitcoin/commit/251d02084688c67523e9ec92ec79ee657454ab93)
I ran all affected help commands and confirmed that all updated outputs are as expected
(https://github.com/bitcoin/bitcoin/pull/33065#issuecomment-3122169008)
tACK [251d020](https://github.com/bitcoin/bitcoin/commit/251d02084688c67523e9ec92ec79ee657454ab93)
I ran all affected help commands and confirmed that all updated outputs are as expected
💬 BitcoinErrorLog commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-3122172886)
> a Bitcoin soft fork should only be implemented in Bitcoin Core after there is (rough) consensus among the Bitcoin developer community
This is not sufficient for inclusion into the most popular implementation.
Developers are not a governance system for Bitcoin. Higher standards must be defined, required, and culturally acceptable.
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-3122172886)
> a Bitcoin soft fork should only be implemented in Bitcoin Core after there is (rough) consensus among the Bitcoin developer community
This is not sufficient for inclusion into the most popular implementation.
Developers are not a governance system for Bitcoin. Higher standards must be defined, required, and culturally acceptable.
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233120018)
Why require users to pass in the converted string rather than letting them just pass in the HRN directly and adding the `.user._bitcoin-payment` ourselves?
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233120018)
Why require users to pass in the converted string rather than letting them just pass in the HRN directly and adding the `.user._bitcoin-payment` ourselves?
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233118375)
This should be allowed, I believe.
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233118375)
This should be allowed, I believe.
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233122778)
spec also mandates that you fail if there is more than one TXT record that starts with "bitcoin:"
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233122778)
spec also mandates that you fail if there is more than one TXT record that starts with "bitcoin:"
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233122382)
I'm not quite sure what the anticipated format for this response is, is it documented anywhere? eg there's the google-proprietary format at https://dns.google/resolve?name=matt.user._bitcoin-payment.mattcorallo.com&type=TXT which seems to be similar to what you want, but uses the integer value for `type` (also would require checking the `AD` bit!).
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233122382)
I'm not quite sure what the anticipated format for this response is, is it documented anywhere? eg there's the google-proprietary format at https://dns.google/resolve?name=matt.user._bitcoin-payment.mattcorallo.com&type=TXT which seems to be similar to what you want, but uses the integer value for `type` (also would require checking the `AD` bit!).
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233123549)
You also need to accept arguments. As @Sjors noted this will be needed for future things like silent payments `?sp=...` but also BIP 321 mandates that you need to recognize any bech32 payment instructions in parameters (eg `bitcoin:1Legacy?bc=segwit&BC=TAPROOT`). Probably this code should be shared with the existing URI-parsing logic in the GUI, which may or may not need updating to handle 321 correctly.
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233123549)
You also need to accept arguments. As @Sjors noted this will be needed for future things like silent payments `?sp=...` but also BIP 321 mandates that you need to recognize any bech32 payment instructions in parameters (eg `bitcoin:1Legacy?bc=segwit&BC=TAPROOT`). Probably this code should be shared with the existing URI-parsing logic in the GUI, which may or may not need updating to handle 321 correctly.
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233124434)
What is the plan here? Ship a separate binary (included via the same multi-process release process) to do resolution or is the goal to eventually just embed something that can talk directly to a DNS server over TCP and fetch results (its pretty trivial code, but its a bunch of parsing to do in C/C++)?
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233124434)
What is the plan here? Ship a separate binary (included via the same multi-process release process) to do resolution or is the goal to eventually just embed something that can talk directly to a DNS server over TCP and fetch results (its pretty trivial code, but its a bunch of parsing to do in C/C++)?
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233122798)
Should be ignoring case.
(https://github.com/bitcoin/bitcoin/pull/33069#discussion_r2233122798)
Should be ignoring case.
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3122198909)
Oh one last comment, it would be nice if the resolver also provided the proof which was stored in the wallet.
In general on-chain bitcoin users expect txids to, by themselves, be "proof of payment". That changes with 353 - you now need the txid plus the DNSSEC proof (as the records may change out from under you). Whether the proof are available today or not, ensuring they get stored in the wallet is good future-proofing for PoP.
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3122198909)
Oh one last comment, it would be nice if the resolver also provided the proof which was stored in the wallet.
In general on-chain bitcoin users expect txids to, by themselves, be "proof of payment". That changes with 353 - you now need the txid plus the DNSSEC proof (as the records may change out from under you). Whether the proof are available today or not, ensuring they get stored in the wallet is good future-proofing for PoP.
🤔 cedwies reviewed a pull request: "test: refactor mempool_accept_wtxid"
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3058474232)
Tested on MacOS 15.5 (M2, Debug build)
- mempool_accept_wtxid.py passes
- Confirmed with grep that build_parent_tx() helper is not referenced anywhere else
- nit: two spaces sneaked into assert_equal(node.getmempoolentry(...), child_one.wtxid_hex) (double space before child_one)
(https://github.com/bitcoin/bitcoin/pull/33067#pullrequestreview-3058474232)
Tested on MacOS 15.5 (M2, Debug build)
- mempool_accept_wtxid.py passes
- Confirmed with grep that build_parent_tx() helper is not referenced anywhere else
- nit: two spaces sneaked into assert_equal(node.getmempoolentry(...), child_one.wtxid_hex) (double space before child_one)