💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481903500)
It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct `TemplateNotifications` and have this line can check `template_notifications.m_interrupt_wait` instead. It's more clear that something called "notifications" can change from under us.
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481903500)
It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct `TemplateNotifications` and have this line can check `template_notifications.m_interrupt_wait` instead. It's more clear that something called "notifications" can change from under us.
💬 TheCharlatan commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3473751931)
> If anyone has documentation showing the format (pre-OP_RETURN) this would be useful.
https://docs.counterparty.io/docs/advanced/protocol/#transactions and https://github.com/CounterpartyXCP/counterparty-core/blob/master/counterparty-core/counterpartycore/lib/api/composer.py#L304 should give an idea. AFAICT the real pubkey goes last.
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3473751931)
> If anyone has documentation showing the format (pre-OP_RETURN) this would be useful.
https://docs.counterparty.io/docs/advanced/protocol/#transactions and https://github.com/CounterpartyXCP/counterparty-core/blob/master/counterparty-core/counterpartycore/lib/api/composer.py#L304 should give an idea. AFAICT the real pubkey goes last.
💬 Sjors commented on pull request "mining: add applySolution() to interface":
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3473753273)
Github completely disappeared the (very useful) earlier feedback from Plebhash. Hopefully his account gets restored, but meanwhile see the @0xB10C backup: https://mirror.b10c.me/bitcoin-bitcoin/33374#issuecomment-3299309843
(https://github.com/bitcoin/bitcoin/pull/33374#issuecomment-3473753273)
Github completely disappeared the (very useful) earlier feedback from Plebhash. Hopefully his account gets restored, but meanwhile see the @0xB10C backup: https://mirror.b10c.me/bitcoin-bitcoin/33374#issuecomment-3299309843
💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3473764682)
@ryanofsky @laanwj Thank you the in-depth discussion here. I previously thought it might be best to leave the default file behavior untouched but if people are unhappy with it now, in this PR, is certainly the right time to fix it and I am not married to the idea of keeping it the same. I have thought about asmap more than most in the last couple of years so I've simply gotten used to how things were.
> The basic problem here is that this option's default value (ip_asn.map) and default behavi
...
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3473764682)
@ryanofsky @laanwj Thank you the in-depth discussion here. I previously thought it might be best to leave the default file behavior untouched but if people are unhappy with it now, in this PR, is certainly the right time to fix it and I am not married to the idea of keeping it the same. I have thought about asmap more than most in the last couple of years so I've simply gotten used to how things were.
> The basic problem here is that this option's default value (ip_asn.map) and default behavi
...
💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3473793738)
[DrahtBot conflict list](https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3190433503) was updated, no more cluster memool conflict, ready for review again!
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3473793738)
[DrahtBot conflict list](https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3190433503) was updated, no more cluster memool conflict, ready for review again!
💬 Sjors commented on pull request "mining: check witness commitment in submitBlock":
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3473794785)
I wrote:
> The Template Provider I implemented takes all sorts of stuff from the coinbase in order to produce its NewTemplate message
This is a continuing source of confusion, see e.g. https://github.com/Sjors/sv2-tp/pull/55#pullrequestreview-3404172536
Orthogonal to this PR, but I plan to add some getters to the interface later.
(https://github.com/bitcoin/bitcoin/pull/33745#issuecomment-3473794785)
I wrote:
> The Template Provider I implemented takes all sorts of stuff from the coinbase in order to produce its NewTemplate message
This is a continuing source of confusion, see e.g. https://github.com/Sjors/sv2-tp/pull/55#pullrequestreview-3404172536
Orthogonal to this PR, but I plan to add some getters to the interface later.
🤔 Crypt-iQ reviewed a pull request: "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer"
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3404930775)
> treat a CMPCTBLOCK announcement as being fine at any time (eg, so that miners who get a block via RPC can just send CMPCTBLOCK messages directly), and the "high-bandwidth" as just a way for nodes to avoid wasting their bandwidth sending those messages unnecessarily
I think yes, except for -blocksonly nodes, where on receipt we wouldn't punish or anything, but we would ideally drop the message?
> change our behaviour when sending GETBLOCKTXN to also request any transactions that were in o
...
(https://github.com/bitcoin/bitcoin/pull/32606#pullrequestreview-3404930775)
> treat a CMPCTBLOCK announcement as being fine at any time (eg, so that miners who get a block via RPC can just send CMPCTBLOCK messages directly), and the "high-bandwidth" as just a way for nodes to avoid wasting their bandwidth sending those messages unnecessarily
I think yes, except for -blocksonly nodes, where on receipt we wouldn't punish or anything, but we would ideally drop the message?
> change our behaviour when sending GETBLOCKTXN to also request any transactions that were in o
...
💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2481900868)
Haven't tested. I think this will cause extra log messages if:
- receive `HEADERS` from LB peer=0
- send `GETDATA` to peer=0
- receive `CMPCTBLOCK` from HB peer=1, it reconstructs, clearing download state for all peers for this block
- receive `CMPCTBLOCK` from LB peer=0
Might confuse a user, but nothing is actually wrong here?
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2481900868)
Haven't tested. I think this will cause extra log messages if:
- receive `HEADERS` from LB peer=0
- send `GETDATA` to peer=0
- receive `CMPCTBLOCK` from HB peer=1, it reconstructs, clearing download state for all peers for this block
- receive `CMPCTBLOCK` from LB peer=0
Might confuse a user, but nothing is actually wrong here?
💬 Crypt-iQ commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2481892837)
Agree that it doesn't make sense to process, we should drop it asap. Haven't tested either of the below things:
`ProcessHeadersMessage` will trigger a `GETDATA` and if the block turns out to be invalid, our peer who sent us the `CMPCTBLOCK` won't respond and will get disconnected for stalling. If our peer is modified to relay these in the first place to all peers, they could isolate themselves from -blocksonly nodes (assuming they aren't a mining node and don't validate what they send out). T
...
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2481892837)
Agree that it doesn't make sense to process, we should drop it asap. Haven't tested either of the below things:
`ProcessHeadersMessage` will trigger a `GETDATA` and if the block turns out to be invalid, our peer who sent us the `CMPCTBLOCK` won't respond and will get disconnected for stalling. If our peer is modified to relay these in the first place to all peers, they could isolate themselves from -blocksonly nodes (assuming they aren't a mining node and don't validate what they send out). T
...
💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3473833357)
For my benchmarking needs `reindex-chainstate` suffices, especially now that I know about this corner-case.
> Log an error that assumevalid is enabled but reindex could not find the specified assumevalid header
Now that https://github.com/bitcoin/bitcoin/pull/33336 was merge we're getting part of that message already which should already help a lot in this situation.
Thanks for taking care of this issue, agree with the concerns of @stickies-v, but I personally would either keep `reindex
...
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3473833357)
For my benchmarking needs `reindex-chainstate` suffices, especially now that I know about this corner-case.
> Log an error that assumevalid is enabled but reindex could not find the specified assumevalid header
Now that https://github.com/bitcoin/bitcoin/pull/33336 was merge we're getting part of that message already which should already help a lot in this situation.
Thanks for taking care of this issue, agree with the concerns of @stickies-v, but I personally would either keep `reindex
...
👍 ryanofsky approved a pull request: "test: ipc: resolve symlinks in `which capnp`"
(https://github.com/bitcoin/bitcoin/pull/33749#pullrequestreview-3405061095)
Code review ACK 51093d6ae1d415b8dfa47f11cb634a87bd97e8a9
I was wondering if this should use `.parent.resolve().parent` instead of `.resolve().parent.parent` but I could imagine resolving first being better if someone has a bin directory with symlinks to installs in different prefixes (like nix or some virtual environments have). There probably isn't a perfect answer but this seems like an improvement.
(https://github.com/bitcoin/bitcoin/pull/33749#pullrequestreview-3405061095)
Code review ACK 51093d6ae1d415b8dfa47f11cb634a87bd97e8a9
I was wondering if this should use `.parent.resolve().parent` instead of `.resolve().parent.parent` but I could imagine resolving first being better if someone has a bin directory with symlinks to installs in different prefixes (like nix or some virtual environments have). There probably isn't a perfect answer but this seems like an improvement.
💬 ryanofsky commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3473896041)
> I have a slight tendency to the first option because some people seem to like the convenience of this feature. What do you think?
I think I like both equally, and it feels like whichever default you pick shouldn't matter too much because it will change again soon when asmap data is embedded? (I'm assuming when asmap data is embedded, the default will be to use embedded data unless you specify -noasmap or a filename.)
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3473896041)
> I have a slight tendency to the first option because some people seem to like the convenience of this feature. What do you think?
I think I like both equally, and it feels like whichever default you pick shouldn't matter too much because it will change again soon when asmap data is embedded? (I'm assuming when asmap data is embedded, the default will be to use embedded data unless you specify -noasmap or a filename.)
🚀 fanquake merged a pull request: "test: ipc: resolve symlinks in `which capnp`"
(https://github.com/bitcoin/bitcoin/pull/33749)
(https://github.com/bitcoin/bitcoin/pull/33749)
⚠️ Sjors opened an issue: "Header-only support for waitNext()"
(https://github.com/bitcoin/bitcoin/issues/33756)
I'd like to expand the `waitNext()` Mining IPC function to optionally (via `BlockWaitOptions`) return a new template as soon as we have a header with enough proof-of-work, pending block validation.
https://github.com/bitcoin/bitcoin/blob/3cd4263bf66408760ec3b4ce6ccedcc87e0d53de/src/interfaces/mining.h#L63-L74
I suspect there's little benefit if compact block reconstruction succeeds, and IIUC we hold cs_main during this process anyway so there's no way to generate a template.
But if we need a
...
(https://github.com/bitcoin/bitcoin/issues/33756)
I'd like to expand the `waitNext()` Mining IPC function to optionally (via `BlockWaitOptions`) return a new template as soon as we have a header with enough proof-of-work, pending block validation.
https://github.com/bitcoin/bitcoin/blob/3cd4263bf66408760ec3b4ce6ccedcc87e0d53de/src/interfaces/mining.h#L63-L74
I suspect there's little benefit if compact block reconstruction succeeds, and IIUC we hold cs_main during this process anyway so there's no way to generate a template.
But if we need a
...
💬 Sjors commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-3473947623)
Possibly relevant: #33756
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-3473947623)
Possibly relevant: #33756
💬 Sjors commented on issue "RFC: Bitcoin Core Node `BlockTemplateManager`":
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3473950791)
Do you think #33756 would be compatible with this?
(https://github.com/bitcoin/bitcoin/issues/33389#issuecomment-3473950791)
Do you think #33756 would be compatible with this?
💬 A-Manning commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2482073588)
Applied the patch, thanks
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2482073588)
Applied the patch, thanks
💬 A-Manning commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2482074480)
Done
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2482074480)
Done
💬 ryanofsky commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482074676)
re: https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481903500
> It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct `TemplateNotifications` and have this line check `template_notifications.m_interrupt_wait` instead. It's more clear that something called "notifications" can change from under us.
I feel like it's not that confusing once you notice it's declared as outside reference (`bool& interrupt_wait`)
...
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2482074676)
re: https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481903500
> It's somewhat unintuitive that we're waiting for a local variable to change. It might be more clear if we make a trivial struct `TemplateNotifications` and have this line check `template_notifications.m_interrupt_wait` instead. It's more clear that something called "notifications" can change from under us.
I feel like it's not that confusing once you notice it's declared as outside reference (`bool& interrupt_wait`)
...
💬 Fi3 commented on issue "Header-only support for waitNext()":
(https://github.com/bitcoin/bitcoin/issues/33756#issuecomment-3473971497)
it make sense for the TP to optimistically create templates as soon as there is a possible new block since this will reduce the time that a miner waste on stale jobs. Is kinda safe since the TP can stop following the optimistic path when 2 or more consecutive invalid blocks are received. If we leave it like it is now the miner will keep mine on the old job until the block template do not provide a new job.
So worst case we waste a bunch of hundreds milliseconds mining invalid jobs, that we are
...
(https://github.com/bitcoin/bitcoin/issues/33756#issuecomment-3473971497)
it make sense for the TP to optimistically create templates as soon as there is a possible new block since this will reduce the time that a miner waste on stale jobs. Is kinda safe since the TP can stop following the optimistic path when 2 or more consecutive invalid blocks are received. If we leave it like it is now the miner will keep mine on the old job until the block template do not provide a new job.
So worst case we waste a bunch of hundreds milliseconds mining invalid jobs, that we are
...