💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1394911231)
Is it on purpose to use `super()` above and provide args here and below? Not an expert on this kind of stuff, but [this](https://stackoverflow.com/questions/59538746/when-do-you-need-to-pass-arguments-to-python-super) suggests that providing args is not necessary in python3.
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1394911231)
Is it on purpose to use `super()` above and provide args here and below? Not an expert on this kind of stuff, but [this](https://stackoverflow.com/questions/59538746/when-do-you-need-to-pass-arguments-to-python-super) suggests that providing args is not necessary in python3.
💬 luke-jr commented on pull request "doc: fixup help output for -upnp and -natpmp":
(https://github.com/bitcoin/bitcoin/pull/28874#discussion_r1394985174)
This loses the "when listening and no -proxy" which is AFAIK still true?
(https://github.com/bitcoin/bitcoin/pull/28874#discussion_r1394985174)
This loses the "when listening and no -proxy" which is AFAIK still true?
💬 denavila commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#discussion_r1394987489)
I added RBF spoofing in the latest commit.
(https://github.com/bitcoin/bitcoin/pull/27792#discussion_r1394987489)
I added RBF spoofing in the latest commit.
💬 luke-jr commented on pull request "doc: fixup help output for -upnp and -natpmp":
(https://github.com/bitcoin/bitcoin/pull/28874#discussion_r1394988966)
```suggestion
argsman.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %s)", !Assert(!DEFAULT_UPNP)), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
```
Otherwise it would be incorrect (missing "when listening and no -proxy") if `DEFAULT_UPNP` flips to true.
Alternatively, just remove the SoftSetArg stuff...
(https://github.com/bitcoin/bitcoin/pull/28874#discussion_r1394988966)
```suggestion
argsman.AddArg("-upnp", strprintf("Use UPnP to map the listening port (default: %s)", !Assert(!DEFAULT_UPNP)), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
```
Otherwise it would be incorrect (missing "when listening and no -proxy") if `DEFAULT_UPNP` flips to true.
Alternatively, just remove the SoftSetArg stuff...
💬 luke-jr commented on pull request "refactor: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1394990462)
This isn't a refactor.
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1394990462)
This isn't a refactor.
🤔 luke-jr requested changes to a pull request: "gui: getrawtransaction implementation"
(https://github.com/bitcoin-core/gui/pull/777#pullrequestreview-1733238681)
Needs a lot of UX work. Definitely doesn't belong on the Help menu...
(https://github.com/bitcoin-core/gui/pull/777#pullrequestreview-1733238681)
Needs a lot of UX work. Definitely doesn't belong on the Help menu...
💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1394993981)
I think you want `fRequireMinimal=true` true here, otherwise it'll accept non-minimally-encoded values too.
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1394993981)
I think you want `fRequireMinimal=true` true here, otherwise it'll accept non-minimally-encoded values too.
💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395001686)
I expect this to be a controversial opinion, but I disagree with calling this "little endian". That's a term that applies to encoding numbers to bytes/bits. Since what's being encoded isn't a number, or to be interpreted as a number, endianness is inapplicable. It's just encoding bytes in hexadecimal format, in order.
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395001686)
I expect this to be a controversial opinion, but I disagree with calling this "little endian". That's a term that applies to encoding numbers to bytes/bits. Since what's being encoded isn't a number, or to be interpreted as a number, endianness is inapplicable. It's just encoding bytes in hexadecimal format, in order.
💬 luke-jr commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395004582)
It's not little endian, I agree.
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395004582)
It's not little endian, I agree.
💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1395005701)
good point let me change the title
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1395005701)
good point let me change the title
👋 kevkevinpal's pull request is ready for review: "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/28885)
(https://github.com/bitcoin/bitcoin/pull/28885)
💬 ajtowns commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1813525342)
> @ajtowns To avoid duplicate work, it would be good if you opened all follow-up pulls you wanted to do, or reply with a list here. Others can then take the remaining follow-ups :)
I think followups are:
* Drop `SER_*` #28451
* Drop version from `GetSerializeSize()` #28878
* Drop CAutoFile - sketch at https://github.com/ajtowns/bitcoin/commits/202311-autofile
* `CVectorWriter` to `VectorWriter`, drop version from `SpanReader`, allow psbt's to be serialized via `DataStream`
* Dro
...
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1813525342)
> @ajtowns To avoid duplicate work, it would be good if you opened all follow-up pulls you wanted to do, or reply with a list here. Others can then take the remaining follow-ups :)
I think followups are:
* Drop `SER_*` #28451
* Drop version from `GetSerializeSize()` #28878
* Drop CAutoFile - sketch at https://github.com/ajtowns/bitcoin/commits/202311-autofile
* `CVectorWriter` to `VectorWriter`, drop version from `SpanReader`, allow psbt's to be serialized via `DataStream`
* Dro
...
💬 luke-jr commented on pull request "doc: fixup help output for -upnp and -natpmp":
(https://github.com/bitcoin/bitcoin/pull/28874#discussion_r1395033114)
`%s` will make `DEAULT_NATPMP` `"false"` here instead of "0", I think...
(https://github.com/bitcoin/bitcoin/pull/28874#discussion_r1395033114)
`%s` will make `DEAULT_NATPMP` `"false"` here instead of "0", I think...
💬 denavila commented on pull request "wallet: Deniability API (Unilateral Transaction Meta-Privacy)":
(https://github.com/bitcoin/bitcoin/pull/27792#discussion_r1395034720)
Hmm, so it doesn't look like feebumper::CommitTransaction would let me bump a transaction if the RBF flag wasn't set on the original transaction. For now I worked around it by passing an extra flag into SpoofTransactionFingerprint to only signal no-rbf when RBF is not required (as is the case with deniabilizaiton transactions). Let me know if you have better ideas how to work around this and take advantage of full-RBF nodes bumping regardless of the flag ...
(https://github.com/bitcoin/bitcoin/pull/27792#discussion_r1395034720)
Hmm, so it doesn't look like feebumper::CommitTransaction would let me bump a transaction if the RBF flag wasn't set on the original transaction. For now I worked around it by passing an extra flag into SpoofTransactionFingerprint to only signal no-rbf when RBF is not required (as is the case with deniabilizaiton transactions). Let me know if you have better ideas how to work around this and take advantage of full-RBF nodes bumping regardless of the flag ...
💬 ajtowns commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#issuecomment-1813628680)
Rebased after #28438 was merged.
> Is this next after #28438? Or something else first?
Either this or #28451 seem good to go; I think they conflict due to the "Convert some CDataStream.." commit here though; shouldn't be a hard rebase in either case though, I think.
> Playing around locally, I was also able to drop the include in:
Taken.
> Also worth pointing out: libbitcoinkernel now only requires `version.h` for `signet.cpp`, which I assume will go away in one of these follow-up
...
(https://github.com/bitcoin/bitcoin/pull/28878#issuecomment-1813628680)
Rebased after #28438 was merged.
> Is this next after #28438? Or something else first?
Either this or #28451 seem good to go; I think they conflict due to the "Convert some CDataStream.." commit here though; shouldn't be a hard rebase in either case though, I think.
> Playing around locally, I was also able to drop the include in:
Taken.
> Also worth pointing out: libbitcoinkernel now only requires `version.h` for `signet.cpp`, which I assume will go away in one of these follow-up
...
👋 ajtowns's pull request is ready for review: "Remove version field from GetSerializeSize"
(https://github.com/bitcoin/bitcoin/pull/28878)
(https://github.com/bitcoin/bitcoin/pull/28878)
💬 ajtowns commented on pull request "contrib: use a raw string for a regular expression literal that contains backslashes in signet/miner":
(https://github.com/bitcoin/bitcoin/pull/28883#issuecomment-1813708851)
utACK defdf67765a3d757f4d3840602eef7ccdac9bb49
(https://github.com/bitcoin/bitcoin/pull/28883#issuecomment-1813708851)
utACK defdf67765a3d757f4d3840602eef7ccdac9bb49
💬 ajtowns commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#issuecomment-1813711869)
Could you make the PR description a standalone statement of what this PR is trying to do, and leave the links to previous comments and questions in a separate comment?
(https://github.com/bitcoin/bitcoin/pull/28885#issuecomment-1813711869)
Could you make the PR description a standalone statement of what this PR is trying to do, and leave the links to previous comments and questions in a separate comment?
💬 petertodd commented on pull request "[WIP] BIP300 (Drivechains) consensus-level logic":
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1813734556)
On Wed, Nov 15, 2023 at 01:05:13PM -0800, Paul Sztorc wrote:
> Peter's "equivocation attack" is just him misunderstanding BMM -- specifically wrongly assuming it is like Namecoin (as he says in his linked article).
>
> Unlike Namecoin, which is its own Blockchain and can survive on its own, BMM assumes that L1 can be found -- the L2 therefore has full access to all of L1 already. L2 already sees everything on L1, and its order.
If that is your design assumption, forcing BMM commitments to be
...
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1813734556)
On Wed, Nov 15, 2023 at 01:05:13PM -0800, Paul Sztorc wrote:
> Peter's "equivocation attack" is just him misunderstanding BMM -- specifically wrongly assuming it is like Namecoin (as he says in his linked article).
>
> Unlike Namecoin, which is its own Blockchain and can survive on its own, BMM assumes that L1 can be found -- the L2 therefore has full access to all of L1 already. L2 already sees everything on L1, and its order.
If that is your design assumption, forcing BMM commitments to be
...
💬 mzumsande commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1813782523)
Ah, I think if we have an unhandled exception like in my previous post, running the test directly (`python3 p2p_v2_earlykeyresponse.py`) will show success, but if we use the test_runner (`python3 test_runner.py p2p_v2_earlykeyresponse.py`) it will show failure, probably because things were written to stderr. So we need to do something about the case I mentioned above.
(https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1813782523)
Ah, I think if we have an unhandled exception like in my previous post, running the test directly (`python3 p2p_v2_earlykeyresponse.py`) will show success, but if we use the test_runner (`python3 test_runner.py p2p_v2_earlykeyresponse.py`) it will show failure, probably because things were written to stderr. So we need to do something about the case I mentioned above.